bugGNU Octave - Bugs: bug #57794, cart2sph, sph2cart: output...

 
 

bug #57794: cart2sph, sph2cart: output argument should not change behavior with different nargout

Submitter:  Nicholas Jankowski <nrjank>
Submitted:  Tue 11 Feb 2020 08:11:56 PM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Matlab Compatibility
Status:  Fixed Assigned to:  None
Originator Name:  Nicholas Jankowski Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Tue 24 Mar 2020 07:48:12 PM UTC, comment #9: 

I reviewed the patch and pushed it to the development branch here https://hg.savannah.gnu.org/hgweb/octave/rev/a23da76e0693.

I made a few modifications.  The '\frac' LaTex command was not supported on my machine so I re-wrote to use '\over' which is what the rest of the manual uses.

The removal of the one output argument for the coordinate functions caused bugs in surfl.m and lightangle.m so I re-did the code there to use the new syntax.

I also moved input validation to be first in the function whenever possible.

Thanks for contributing the patch.  Marking as fixed and closing report.

Rik <rik5>
Group administrator
Mon 17 Feb 2020 01:34:34 AM UTC, comment #8: 

ok. cleaned up patch attached.  fixed 2 tests also. i grepped the source tree for any function calls and the ones that showed up all called these functions using the multiple-input format. none relied on the single matrix input/output. those I could test (by moving these functions to that folder) I did so and they passed or the demo could run. tried to clean up the patch according to the wiki patch guidelines as well.



(file #48423)

Nicholas Jankowski <nrjank>
Group Member
Mon 17 Feb 2020 12:34:10 AM UTC, comment #7: 

probably not as big as it looks. Some logic flow changes to the input checking that's the same across all four functions, but looks like complete function rewrites and a couple BISTs added to each one as well.

editor: for m-files, octave editor. for doc files, notepad++. I found a command for that in notepad++ and set it. Don't see anything similar in octaves editor. but I can get in the habit of doing a final save in notepad++. Will upload a cleaned up version shortly.

Nicholas Jankowski <nrjank>
Group Member
Sun 16 Feb 2020 10:12:24 PM UTC, comment #6: 

This is a big, and probably very useful change. I'll try to apply, test, and review it when I get some time, but it will take a little more time investment than I have right now.

At a glance, I see that the patch contains quite a few lines with trailing spaces. What editor did you use to make these changes? Can it be configured to delete trailing spaces when saving files?

Do the tests and demos in contour.m, contourf.m, and voronoi.m still work with these changes?

Mike Miller <mtmiller>
Group Member
Thu 13 Feb 2020 07:38:58 PM UTC, comment #5: 

patch attached with file and NEWS update.

(file #48406)

Nicholas Jankowski <nrjank>
Group Member
Thu 13 Feb 2020 07:37:58 PM UTC, comment #4: 

ok, m-files attached. patch pending.

(file #48402, file #48403, file #48404, file #48405)

Nicholas Jankowski <nrjank>
Group Member
Thu 13 Feb 2020 04:19:25 AM UTC, comment #3: 

i worked up a compatible cart2sph (attached).  the output is never an array. it returns t, p, and r according to the number of outputs requested.

Rereading the original help text, I think I was overthinking the statement "each row of C represents the Cartesian coordinate (x, y, z)".  I had interpreted the text to mean row 1 was x, row 2 was y, and 3 was z.  What I guess it means is each row is an independent ordered triplet (x,y,z).

The revised function allows 3-input arrays to be any dimension as long as they all match, and a single matrix input follows the convention above. It must have three columns, one for x, y, and z, but any number of rows.

Note that one alternative would be to always create a matrix output for a matrix input, but it was simpler to keep it the way it's described above.

Expanded the help text a bit to to clarify the coordinate convention and transformation used.

I'll take a peek at sph2cart, as well as the pol2cart/cart2pol functions a bit later then work up a patch.

Nicholas Jankowski <nrjank>
Group Member
Wed 12 Feb 2020 07:06:46 PM UTC, comment #2: 

poking around at the function, both matlab and octave accept arrays for x, y, and z.  this is easy enough to handle when they're independent via elementwise operations. However, if we want to keep Octave's ability to handle a single matrix input, an input orientation has to be defined for n>1.  otherwise there's no way to tell what is meant by an input of:

C= [0 0 0;
    1 1 1;
    2 2 2];


Nicholas Jankowski <nrjank>
Group Member
Wed 12 Feb 2020 07:34:02 AM UTC, comment #1: 

Confirmed, the positional output arguments should remain the same no matter how many output arguments the function is called with.

The same applies to the inverse function sph2cart.

The current behavior does match the help text, but it doesn't matter because it's going to change anyway.

Mike Miller <mtmiller>
Group Member
Tue 11 Feb 2020 08:11:56 PM UTC, original submission:  

the standard form for calling cart2sph [1] is

 [theta, phi, r] = cart2sph (x, y, z)

If called this way, Octave's output is Matlab compatible.  Similarly, if called with only two outputs, both programs will output only theta and phi.  however, if either zero or one output is specified, the behavior is incompatible:

Octave 5.1.0:

>> [a,b,c] = cart2sph(1,2,3)
a =  1.1071
b =  0.93027
c =  3.7417

>> [a,b] = cart2sph(1,2,3)
a =  1.1071
b =  0.93027

>> a = cart2sph(1,2,3)
a =

   1.10715   0.93027   3.74166

>> cart2sph(1,2,3)
ans =

   1.10715   0.93027   3.74166


Matlab 2019a:

>> [a,b,c] = cart2sph(1,2,3)
a =
    1.1071
b =
    0.9303
c =
    3.7417
>> [a,b] = cart2sph(1,2,3)
a =
    1.1071
b =
    0.9303
>> a = cart2sph(1,2,3)
a =
    1.1071
>> cart2sph(1,2,3)
ans =
    1.1071


Whether or not this incompatibility is deliberate, it could cause mismatch errors for anyone trying to run code on both systems.

Second, the Octave behavior does not match the help text, which specifies that:

"If only a single return argument is requested then return a matrix S where each row represents one spherical coordinate (theta, phi, r)."

The output appears to be a single row with each column representing one spherical coordinate.


Finally, Octave allows an input form that Matlab does not, where the input is a single 3 element array where "each row of C represents the Cartesian coordinate (x, y, z)".  However:


>> [a,b,c] = cart2sph([1,2,3])
a =  1.1071
b =  0.93027
c =  3.7417

>> [a,b,c] = cart2sph([1;2;3])
error: cart2sph: matrix input must have 3 columns [X, Y, Z]
error: called from
    cart2sph at line 52 column 7


Again, it appears that the row/column behavior is reversed from the text.

I would suggest the output behavior be made matlab compatible (only returning the first value), and a row or column determination be made for the input. Or, better, I see no reason the function should not accept both a row or column array, so long that input element order is preserved.

(there was also recently some concern [2] that the help text could be more explicit in using the terms elevation and azimuth, since there are a number of conventions for using the variables theta and phi)

[1] https://octave.sourceforge.io/octave/function/cart2sph.html
[2] https://octave.1599824.n4.nabble.com/use-of-cart2sph-not-in-line-with-manual-tp4695664.html

Nicholas Jankowski <nrjank>
Group Member

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #48424:  cart2sph_v2.m added by nrjank (6KiB - text/plain - updated functions)
file #48425:  sph2cart_v2.m added by nrjank (6KiB - text/plain - updated functions)
file #48426:  cart2pol_v2.m added by nrjank (7KiB - text/plain - updated functions)
file #48427:  pol2cart_v2.m added by nrjank (8KiB - text/plain - updated functions)
file #48423:  coordXform_fix_v2.diff added by nrjank (31KiB - application/octet-stream - cleaned up + two BIST fixes)
file #48406:  coordXform_fix.diff added by nrjank (31KiB - application/octet-stream)
file #48402:  cart2pol.m added by nrjank (7KiB - text/plain - modified coord transform m files)
file #48403:  cart2sph.m added by nrjank (6KiB - text/plain - modified coord transform m files)
file #48404:  pol2cart.m added by nrjank (8KiB - text/plain - modified coord transform m files)
file #48405:  sph2cart.m added by nrjank (6KiB - text/plain - modified coord transform m files)
file #48398:  cart2sph.m added by nrjank (6KiB - text/plain)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by nrjank (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 group members can vote.

     

    Follow 17 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2020-03-24 rik5 StatusPatch Submitted Fixed
        Open/ClosedOpen Closed
    2020-02-17 nrjank Attached File- Added cart2sph_v2.m, #48424
        Attached File- Added sph2cart_v2.m, #48425
        Attached File- Added cart2pol_v2.m, #48426
        Attached File- Added pol2cart_v2.m, #48427
    2020-02-17 nrjank Attached File- Added coordXform_fix_v2.diff, #48423
    2020-02-16 mtmiller StatusConfirmed Patch Submitted
    2020-02-13 nrjank Attached File- Added coordXform_fix.diff, #48406
    2020-02-13 nrjank Attached File- Added cart2pol.m, #48402
        Attached File- Added cart2sph.m, #48403
        Attached File- Added pol2cart.m, #48404
        Attached File- Added sph2cart.m, #48405
    2020-02-13 nrjank Attached File- Added cart2sph.m, #48398
    2020-02-12 mtmiller StatusNone Confirmed
        Release5.1.0 dev
        Summarycart2sph output size is incompatible with matlab depending number of outputs cart2sph, sph2cart: output argument should not change behavior with different nargout

    Back to the top

    Powered by Savane 3.13-758e.
    Corresponding source code