patchGNU Octave - Patches: patch #8983, automatic broadcasting for...

 
 

patch #8983: automatic broadcasting for assignment operations

Submitter:  Carnë Draug <carandraug>
Submitted:  Sat 16 Apr 2016 07:25:11 PM UTC
   
 
Category:  Core : new feature Priority:  5 - Normal
Status:  None Privacy:  Public
Assigned to:  None Open/Closed:  Open
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Tue 19 Apr 2016 11:16:44 AM UTC, comment #12: 

Michael, in comment #7, you mentioned
  warning ("error", "Octave:broadcast");

IIRC the old warning ID removed from 4.0.0 was "Octave:automatic-broadcast".

An unfortunate side-effect of Octave allowing user-defined warnings is that no warning is given if a non-existent warning is specified -- I suspect the line you add to you .octaverc doesn't help.

Lachlan Andrew <lachlan>
Tue 19 Apr 2016 11:03:25 AM UTC, comment #11: 

Carnë, I don't mind too much which tracker you use.  It was just a thought, since IIRC you had previously mentioned that you sometimes file bug reports as a reminder to yourself to do things when you get around to it.

Thanks for the background on "local".  I knew that it existed, but hadn't explored its specific semantics.  Having something that is specific to the scope of the caller is a great idea!

I've started implementing a "local_only" mode in which a warning only applies to the current stack frame.  My motivation is Octave:language-extension, and that raises a big problem: the function is parsed in the stack frame of its first caller.  Code such as


function foo
  warning ("on", "Octave:language-extension");
  a = 1;
  a++;
end


doesn't raise a warning, even without "local" or "local_only".

More importantly, even if "local_only" only applies to the current function, then


function bar
  warning ("on", "Octave:language-extension", "local_only");
  ls


will raise warnings in the Octave function  ls.

That is probably the best reason to separate Octave:automatic-broadcast back out from Octave:language-extension -- the former should be active in the code that is to be Matlab-compatible, and the latter should be active only in the caller of that code.  Perhaps the parse-time warnings should be triggered by a "pragma" in the file being parsed rather than a warning status, and the latter be used for execution-time warnings.

Anyway, that is a debate we can have once the local_only code is ready.

Lachlan Andrew <lachlan>
Sat 16 Apr 2016 07:30:39 PM UTC, comment #10: 

@lachlan

I take back what I said.  This option to move items to another tracker seems duplicates the whole discussion :/  (and on top of that, seems I messed up when selecting the tracker to move this)


Most people will either add a bug or patch report and not think much about it.  Many of them will only do it once and you can't tell them in advance about where they should report it  Telling them to open it again on the other tracker is not very productive.  Since you can't move items between trackers, you will end up with wrong type of reports on each tracker. So having different trackers will actually make things harder to triage since you will have to search on all.

But you're doing a lot of work with the triaging so whatever you feel works best.  I'm happy either way, just let me know.

Carnë Draug <carandraug>
Group Member
Sat 16 Apr 2016 07:25:11 PM UTC, comment #9: 

This item has been reassigned from the project GNU Octave bugs tracker to your tracker.

The original report is still available at bugs #47687

Following are the information included in the original report:

[field #0]                  Item ID: 47687
[field #1]                 Group ID: 1925
[field #2]              Open/Closed: Open
[field #3]                 Severity: 1 - Wish
[field #4]                  Privacy: Public
[field #9]                 Category: Interpreter
[field #10]             Submitted by: carandraug
[field #11]              Assigned to: None
[field #12]             Submitted on: Wed 13 Apr 2016 12:01:37 GMT
[field #13]                  Summary: automatic broadcasting for assignment operations
[field #14]      Original Submission: Just like we support assignment to arrays with a scalar:


a = zeros (3, 4);
a(:,:) = 5;


It would be nice we could also broadcast for assignment, like so:


a = zeros (3, 4);
a(:,:) = [0:3];


Carnë Draug <carandraug>
Group Member
Sat 16 Apr 2016 07:24:58 PM UTC, comment #8: 

@Lachlan

> [...] When you distinguish "local" from "the scope of the function", what is the difference? Does "local" mean just the current stack frame and "scope" mean the stack frame and all descendants?


Indeed, local usually means local to the function.  But some years ago, warning has an option named "local" which only affects the current stack frame and downstream.  You are well aware that warning states are global, but if you don't know about the "local" option", see the effect here:


function trigger_warn ()
  5 / 0;
endfunction

function foo ()
  disp ("trigger warning in subfunction");
  trigger_warn ();
  disp ("'locally' disable warning and cause warning in sub-function");
  warning ("off", "Octave:divide-by-zero", "local");
  trigger_warn ();
endfunction

warning ("query", "Octave:divide-by-zero")
foo ()
warning ("query", "Octave:divide-by-zero")


Which generates:


octave> warning ("query", "Octave:divide-by-zero")
ans =

  scalar structure containing the fields:

    identifier = Octave:divide-by-zero
    state = on

octave> foo ()
trigger warning in subfunction
warning: division by zero
warning: called from
    trigger_warn at line 2 column 5
    foo at line 3 column 3
'locally' disable warning and cause warning in sub-function
octave> warning ("query", "Octave:divide-by-zero")
ans =

  scalar structure containing the fields:

    identifier = Octave:divide-by-zero
    state = on


This reduced the use of unwind_protect blocks.  The use of word "local" for this option was unfortunate (there are several other like this, try 'lookfor ("-all", '"local"')').

So when I said 'scope of the caller only (different from the "local" option in warning)', I mean only the current stack frame.

> [...] suggesting "underline /", "underline *" etc for operations with broadcast. Any other combination of symbols would be fine. [...] Was that discussed at OctConf too?


No, only whether we should place the warnings back or remove the language extension warning.

> On an unrelated note, how would you feel about putting these "to-do" items in the task list, instead of the bug list?


I wrote a long text explaining that was not good because you can't move items between trackers.  Seems Like I was wrong.  The only thing I'll point out is that I feel less people subscribed to the task and patch tracker (based on the reduced number of replies I get when I report there).  I'll move this to the task tracker.

Carnë Draug <carandraug>
Group Member
Sat 16 Apr 2016 12:15:44 AM UTC, comment #7: 

lachlan: In comment 3 you asked about catching
braodcasts. I set something like:
warning ("error", "Octave:broadcast");
in my .octaverc depending on what I am
editing.

Michael Godfrey <godfrey>
Group Member
Fri 15 Apr 2016 12:45:31 AM UTC, comment #6: 

On an unrelated note, how would you feel about putting these "to-do" items in the task list, instead of the bug list?

The task list is quite underused, and triage is easier if wishlist items are separated from "it doesn't work" bugs.

Lachlan Andrew <lachlan>
Fri 15 Apr 2016 12:42:42 AM UTC, comment #5: 


> So unless Octave gets a new way to set warnings within the scope
> of the caller only (different from the "local" option in
> warning), such warning will be of quite limited use.


Developing a new warning system is on my "pipe-dream to-do list", mainly motivated by our previous discussion on this topic.  When you distinguish "local" from "the scope of the function", what is the difference?  Does "local" mean just the current stack frame and "scope" mean the stack frame and all descendants?


I'm not yet convinced that the usefulness of a broadcast warning would be all that limited.  Compare the number of core functions that use broadcasting with the number that use '#' for comments...

Anyway, I think a much better solution would be to replace "automatic" broadcasting with explicit broadcasting with a non-intrusive syntax.  The underlines in comment #3 were missed.  I as suggesting "underline /", "underline *" etc for operations with broadcast.  Any other combination of symbols would be fine.  (It is a pity that Octave uses ! and # to duplicate Matlab symbols, so there are fewer for new language features, but that's another discussion.)  Was that discussed at OctConf too?

Lachlan Andrew <lachlan>
Thu 14 Apr 2016 03:42:40 PM UTC, comment #4: 


> Why would this be nice?
>
> It is easy to use repmat for this situation.


Exactly because it avoids using repmat, and will perform faster.  This is exactly the same as numpy.

> As I have previously pointed out, automatic broadcasting masks many programming errors making debugging harder. Remember that the size of a may have been set in an entirely different function.


Isn't that the beauty of it?  You don't know the size of "a" so you don't have to worry about expanding "b" yourself.  And while you don't know the size of "a", you do know what it represents, and what each of its dimensions are.

> If you saw the line
>
> a(:,:) = b;
>
> in isolation, you would have no way of knowing whether or not broadcasting had been applied, or to what dimension. If you expected b to be a scalar, it would be nice for the system to warn you that it wasn't -- but we can't because we can't set warnings for automatic broadcast.


Actually, if "b" is a scalar this is automatic broadcasting.  It is expanding "b" singleton dimensions as necessary.

> Also notice that in all other cases, automatic broadcasting is symmetric in the dimensions of the two arguments. Would you expect
>
> a(1, 1:2) = eye (2);
>
> to give you a 1x2 or 2x2 result, or an error?


An error.  Assignment is not a symmetric operation, you can only expand the RHS.

> In our last discussion, you said that there should be no warning for automatic broadcasting because "it is part of the language".


It was not that straightforward but yes.  This then got further discussed in person during OctConf, including placing that warning back.  The argument is that automatic broadcasting is part of the language and it has an isue with Matlab incompatibility, therefore it goes in the same warning as all other language extensions.  But there is a real case to be made about having it as a separate error to prevent other errors (although maybe that only happened in the beginning, I haven't seen many error reports about it in a long while).

It has also been discussed elsewhere that the whole language-extensions warning is useless and I agree (we might as well just get rid of it. Octave is unusable with that warning enabled).  But that is an issue how warnings work in the language and it's required for Matlab compatibility.  Say we do make a separate warning for broadcast.  Normal use of Octave will be throwing warnings because octave core and packages do make use of broadcast on purpose.

So unless Octave gets a new way to set warnings within the scope of the caller only (different from the "local" option in warning), such warning will be of quite limited use.

Carnë Draug <carandraug>
Group Member
Thu 14 Apr 2016 04:22:30 AM UTC, comment #3: 

Michael, what default warning are you talking about?  AFAICT, there was a warning in 3.8.2 but that warning was removed in 4.0.0.

More specifically, the warning was merged into "Octave:language-extension", which is thrown so frequently by Octave's own scripts as to be unusable.

I have argued that it should be reinstated as a separate warning, but Carnë argued strongly against that, on the basis that it is "part of the language", as I mentioned below.

I entirely agree that broadcasting is useful, and it would be great to have new operators to do it more cleanly that bsxfun (akin to ./ .* etc, we could have / * etc or some such).  I just don't like the fact that it is compulsorily overloaded onto common operators.

Lachlan Andrew <lachlan>
Wed 13 Apr 2016 11:48:33 PM UTC, comment #2: 

Broadcasting is useful in cases where dimensions may
validly vary according to some requirements, such as
bus widths for example.

But, its accidental use can be dangerous and confusing.
Thus, the default warning is appropriate.
That is the way I use it.

Michael Godfrey <godfrey>
Group Member
Wed 13 Apr 2016 09:57:26 PM UTC, comment #1: 

Why would this be nice?

It is easy to use repmat for this situation.

As I have previously pointed out, automatic broadcasting masks many programming errors making debugging harder.  Remember that the size of  a  may have been set in an entirely different function.  If you saw the line


a(:,:) = b;


in isolation, you would have no way of knowing whether or not broadcasting had been applied, or to what dimension.  If you expected  b  to be a scalar, it would be nice for the system to warn you that it wasn't -- but we can't because we can't set warnings for automatic broadcast.

Also notice that in all other cases, automatic broadcasting is symmetric in the dimensions of the two arguments.  Would you expect


a(1, 1:2) = eye (2);


to give you a 1x2 or 2x2 result, or an error?

In our last discussion, you said that there should be no warning for automatic broadcasting because "it is part of the language".  If it is spreading further and further, then I'm tempted to compare it to a cancerous part of the language.

Unless there is a clear instance when this is useful, I will argue strongly against it.

Lachlan Andrew <lachlan>
Sat 16 Apr 2016 07:25:11 PM UTC, original submission:  

Just like we support assignment to arrays with a scalar:


a = zeros (3, 4);
a(:,:) = 5;


It would be nice we could also broadcast for assignment, like so:


a = zeros (3, 4);
a(:,:) = [0:3];


Carnë Draug <carandraug>
Group Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

No files currently attached

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by godfrey (Posted a comment)
  • -email is unavailable- added by lachlan (Posted a comment)
  • -email is unavailable- added by carandraug (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only logged-in users can vote.

     

    Follow 2 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2016-07-07 lachlan CategoryNone Core : new feature
    2016-04-16 carandraug Reassign itemGNU Octave, bug #47687 GNU Octave, patch #8983

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code