patchGNU Octave - Patches: patch #9953, [octave forge] (mapping) angl2str

 
 

patch #9953: [octave forge] (mapping) angl2str

Submitted by:  Ricardo Fantin da Costa <ricardofantin>
Submitted on:  Sat 11 Jul 2020 07:22:58 PM UTC  
 
Category:  Forge : new function Priority:  5 - Normal
Status:  Done Privacy:  Public
Assigned to:  Philip Nienhuis <philipnienhuis> Open/Closed:  Closed

Add a New Comment (Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

( Jump to the original submission)

Mon 07 Sep 2020 08:38:35 PM UTC, comment #23: 

Some basic fixes here:
http://hg.code.sf.net/p/octave/mapping/rev/acb8b8d6ff0c

  • prefix "{" and "}" in texinfo with "@"
  • just to be sure replace "\" with a macro in texinfo
  • double space after a full stop in texinfo
  • a few typos
  • replace ' with " where possible
  • break long lines
  • move some in-line comments on their own line.

That'll do for the day, no need to make it my code :-)

Philip Nienhuis <philipnienhuis>
Project MemberIn charge of this item.
Mon 07 Sep 2020 07:37:50 PM UTC, comment #22: 

I pushed angl2str.m here:
http://hg.code.sf.net/p/octave/mapping/rev/533ab1868975

It turns out that I accidentally already pushed most of the BIST tests you made earlier on, apologies.

Thanks very much for your contribution, Ricardo, and sorry for a bit of going hence-and-forth with various versions. That was mostly meant to avoid having to be relying on a new "round" function in core Octave.

That said, I do hope you want to pursue that round.m patch - will you please open a separate patch report for that?

Closing report.  Yet I'll fix a few style things and texinfo issues:

>> help angl2str
C:\Users\philip\AppData\Local\Temp\octave-help-LJC4dD:143: misplaced {
C:\Users\philip\AppData\Local\Temp\octave-help-LJC4dD:143: misplaced }
C:\Users\philip\AppData\Local\Temp\octave-help-LJC4dD:143: misplaced {
C:\Users\philip\AppData\Local\Temp\octave-help-LJC4dD:143: misplaced }
warning: help: Texinfo formatting filter exited abnormally; raw Texinfo source of help text follows...

... later on (no worries, texinfo errors can be hard to track down, I'll take care of those), I'll put the link in a later comment here.

Philip Nienhuis <philipnienhuis>
Project MemberIn charge of this item.
Mon 07 Sep 2020 12:50:00 AM UTC, comment #21: 

Hello.
My name can be in commit message. I tell everyone about my contributions in free software.

Ricardo Fantin da Costa <ricardofantin>
Fri 04 Sep 2020 10:27:37 AM UTC, comment #20: 

OK, fine.

Any objections to being credited for this patch (i.e., your name in the commit message?  (I'm asking because some contributors explicitly want to remain anonymous.)

I'll first push your version then (just with copyright string updated to include 2020), later on I'll push a few (minor) style fixes.

Philip Nienhuis <philipnienhuis>
Project MemberIn charge of this item.
Thu 03 Sep 2020 11:03:46 PM UTC, comment #19: 

I tested the new roundn with numbers with imaginary part and everything worked as expected.

I do not have a personal project using angl2str to test. I start coding for octave just to collaborate with code to a project, for a long time I helped free softwares only answering questions in forums and e-mail lists even knowing how to program.

I think everything is finished and ready to be pushed.

Ricardo Fantin da Costa <ricardofantin>
Thu 03 Sep 2020 08:45:15 PM UTC, comment #18: 

...changed sign of the second argument...
==>
...changed sign of the second argument in calls to roundn...

Philip Nienhuis <philipnienhuis>
Project MemberIn charge of this item.
Thu 03 Sep 2020 08:44:20 PM UTC, comment #17: 

Looking at roundn.m and playing around a little, I see that replacing subfunction round_a as follows

function [num] = round_a (X, N = 0)

##  if (N == 0)
##    num = floor (X + 0.5);
##  else
##    multiplier = 10 .** (N);
##    num = (floor ((X .* multiplier) + 0.5)) ./ multiplier;
##  endif

  num = round (X / 10^-N) * 10^-N;

endfunction

does the trick. That is, with that change all 491 tests of your latest version pass.

Now, looking at roundn.m and comparing with Matlab, I conclude that replacing its working part

:
  if (mod (x, 1) != 0)
    ret = round (10^abs (n) * x) / (10^abs (n));
  else
    ret = round (x / 10^abs (n)) * 10 ^ abs (n);
  endif
:

by just

:
  ret = round (x / 10^n) * 10^n;
:

makes it fully Matlab-compatible, AFAICS.

So in angl2str.m I replaced all calls to round_a with calls to roundn, deleted subfunction round_a, and changed sign of the second argument, and again all 491 tests pass.

I upload that patched angl2str.m and the patched roundn.m; can you confirm my analysis and check that anglstr.m works for you as well (outside from the BIST tests, just with your project(s))?

If yes I'll check texinfo, style etc. and then I'll push it.

(file #49738, file #49739)

Philip Nienhuis <philipnienhuis>
Project MemberIn charge of this item.
Thu 03 Sep 2020 06:09:34 PM UTC, comment #16: 

I agree that roundn.m needs to be fixed as well. Maybe I can give it a try integrating the relevant parts of your round.m code into roundn.m.

Hang on ...

Philip Nienhuis <philipnienhuis>
Project MemberIn charge of this item.
Wed 02 Sep 2020 09:07:21 PM UTC, comment #15: 

Thanks, hopefully tomorrow night or Friday I'll have time for further review.

Philip Nienhuis <philipnienhuis>
Project MemberIn charge of this item.
Wed 02 Sep 2020 06:32:30 PM UTC, comment #14: 

Update mostly style things:
Long lines divided in two or three. It apply to comments, string
concatenations and calculations. Some lines passed a little the 80 char length.
Lines with only comments uses two # instead of one #.
Use of ismember() instead of some strcmp().
Verification of arguments types made before others verifications.
Function round() was included as an inner function renamed to round_a().
I tested the use of roundn(), but probably there is one bug in that function.

I do not know English well to find grammar errors. I need your help in this part.

The -181.6999999999 that did not became a 181° 42' 00.0000" W because of a || that should be a | .
Code before:
m(s >= 60 || (s == 0 && intermediary_calc >= 30)) += 1;
Code now:
m(s >= 60 | (s == 0 & intermediary_calc >= 30)) += 1;
The error only occur when at least two numbers are inputted, and most tests had only one input each time. Your sample became a test (the 491º test case).

I found a bug in roundn(). If was not for the bug, then roundn would be exactly what I need. roundn(20,-2) should became 20.00, but it turns into 0.

roundn() is the same thing that last version round(), but without the option to round using "significant". In roundn() where is wrote:
if (mod (x, 1) != 0)
should verify if n is positive or negative. mod (x,1) verifies if x has a fractional part, I did not understand this verification intention and I may be interpreting wrong.

Could you take a look in the code again? (grammar, inner function round_a instead of round or roundn, some style fault)

(file #49734)

Ricardo Fantin da Costa <ricardofantin>
Wed 02 Sep 2020 06:16:16 PM UTC, comment #13: 

I've brushed up my angl2str.m version, renamed it to angltostr.m and added a provisional angl2str.m wrapper that for the time being calls angltostr.m behind the scenes.
Hopefully we can soon drop the wrapper and get your version in place then.

My motives are that IMO Matlab's angl2str is actually little more than a kludge. It doesn't fit my needs and needs significant pre- and post processing to get acceptable (IMO) results [*]. angltostr.m does a better job, although I already saw some issues :-)

[*] Some mentioned in comment #4: no wrapping into [-180, 180] or maybe [0, 360], or [-90 90] for latitudes. I've never seen use cases for value outside those ranges.
In addition Matlab's help explicitly mentions input in degrees only so I'd expect radians output to be converted from degrees, but actually there's no conversion at all, angl2str actually only does formatting.
char arrays are also a pain often, cellstr arrays are much easier to work with.
I'll probably file a bug report at TMW, but in general the outcome of that is rarely satisfying IMO.

Philip Nienhuis <philipnienhuis>
Project MemberIn charge of this item.
Sun 30 Aug 2020 09:16:02 AM UTC, comment #12: 

Fine.

status=> In Progress

Philip Nienhuis <philipnienhuis>
Project MemberIn charge of this item.
Sun 30 Aug 2020 01:41:57 AM UTC, comment #11: 

Hello.

I will solve these problems. May take a while, probably less than a week, but no more than a month.

Ricardo Fantin da Costa <ricardofantin>
Sat 29 Aug 2020 09:42:41 PM UTC, comment #10: 

Hi Ricardo,

My pleasure.

It's not that have something to commit ("push") yet, just your version with your round.m absorbed as subfunction with input validation commented out; plus a version with only some stye fixes yet, more to come in the next days.

It just occurred to me that there's also roundn.m in the mapping package - could you please check if that fits your needs for anglstr.m rather than round.m? If yes the code could be even simpler as round.m wouldn't be needed at all then.

Furthermore I just saw an issue:

>> angl2str ([-181.6999; -181.699999; -181.6999999999; -181.7; 181.71], 'ew', 'degrees2dms', -4)
ans =
181° 41' 59.6400" W
181° 41' 59.9964" W
181° 41' 00.0000" W
181° 41' 00.0000" W
181° 42' 36.0000" E

where in the steps from -181.699999 .. -181.7 some erroneous wrapping occurs; somewhere in that sequence I'd expect minutes to go to 42, not stay 41. No doubt this is related to rounding somewhere, but whatever the cause it is wrong.
I just checked with Matlab and that yielded a remarkable result for -181.7: while it gets the total right as it returns <degrees> 41' 60", it should of course round to 42' 00". Hmmm, wonky rounding as well there ...

Hint:
To check this sort of rounding stuff around critical values like 180 or 360 degrees, or 0 (zero), etc., I usually enter a small input range around such values with a tiny increment along the lines of:

output = <function> ([start : 1e-9 : end], < rest of input args>);

and then check for monotony in the output (min/max (diff (output)), for strings usually after applying some regexps & str2double & optional postprocessing. You'd be surprised to see how many problems sometimes turn up then :-)
(but in the above case I didn't even get that far, I was just trying a few values to compare with Matlab.)

Could you have a look at these two things, please? (roundn.m and that accuracy thing)

Philip Nienhuis <philipnienhuis>
Project MemberIn charge of this item.
Sat 29 Aug 2020 08:05:58 PM UTC, comment #9: 

Thank you Philip. Your way to do is the best. If I understand correctly you already have a working angl2str ready to commit and I just want to say yes to this version. When the code get to forge I will see the final version and learn the standards. I'm understanding better the C++ code and eventually will submit in another patch an improved round(). When new round() get accepted I will let you know and we just wait a few years to change angl2str.

It was a very pleasant experience. There are some other patches and codes that I need to improve before make new patches to mapping.

Ricardo Fantin da Costa <ricardofantin>
Sat 29 Aug 2020 01:37:56 PM UTC, comment #8: 

@ricardofantin:
I've just put your round.m as subfunction "round_a" into your angle2str.m + replaced the calls and all 490 tests pass.

That doesn't inhibit your procedure to improve round.m in core octave, it's just meant to get a working angl2str.m into mapping without the issues outlined in comment #7.
(indeed Octave's round.m can be improved. I'll support a separate patch for that but I'm not the one to decide about it :-) )

If you agree with this "fix" for round we can incorporate your angl2str.m into the mapping package that way and replace the one written by me (no problems :-) yours is much more Matlab-compatible). In that new subfunction "round_a" all input validation can be dropped as well.

If the rounding you need can be done in an alternative way (I think that's possible) it can be tackled later on, maybe even in some much later mapping package release. As pkg maintainer I value correct output, Matlab compatibility and complete package functionality (in that order) somewhat higher than speed and "superior" coding (which is always a subjective interpretation). IOW "first get it in then it can be improved later on".

That said, I spotted a few style & logic issues but I can tackle those after first committing your angl2str.m. I mention them here just FYI:

  • print_usage after an error statement. print_usage() never gets called then as execution breaks at the preceding error statement. In Octave print_usage is rarely if ever used anyway when checking input arg contents and your suggested error messages are sufficiently informative, if somewhat verbose.
  • tolower => lower
  • missing line continuation markers esp. in compound if statements. Sometimes this can give rise to hard to uncover bugs
  • input validation should first check type, e.g., just try 'angl2str (5, 5)' or 'angl2str (5, "ew", 5)'.
  • double rather than single quotes here and there
  • too long lines
  • switch statements should include an 'otherwise' clause
  • precede comment lines with '##' rather than single '#'
  • + a few typos.

So, what do you think?

Philip Nienhuis <philipnienhuis>
Project MemberIn charge of this item.
Sat 29 Aug 2020 11:29:49 AM UTC, comment #7: 

I started reviewing your angl2str in some more detail.

Can you explain why round.m needs to patched in the first place?
If possible I'd like to avoid having that patched round.m either in core or in the mapping package for the following reasons:

If in core Octave

  1. First of all the usual prudency arguments for changing anything in core; no one knows what code Out There depends on current round.m functionality, let alone some core Octave functions
  2. The mapping package and core Octave have different release cycles
  3. The mapping package should work with older Octave versions for a number of years after they were released. So for older Octave versions a new mapping package's anglstr wouldn't work
  4. If in core it'll be released only next year (we hope), so until then the mapping package can't use it
  5. In short, we'd need a workaround anyway for the next years in the mapping package itself.

If in the mapping package

  • When loading the mapping package, its entire function directory is placed before all of core Octave so mapping's round.m would shadow core Octave's version. So there you go, back to point 1 above ...
  • There may be other mapping package functions, if not other OF package functions, affected by a change in round.m's functionality (I haven't looked yet for the mapping package, for other OF packages I won't even attempt)

If your angl2str really needs an amended round.m it had better be a subfunction of angl2str.m to avoid all above mentioned scoping issues. In addition it rather be renamed to something like round_a or so to avoid any risk of function name mix-up.

But maybe / hopefully there are other ways to get numbers rounded properly for angl2str w/o invoking round.m at all. I'm all prepared to help sorting that out but then I need to know exactly what kind of rounding you need for angl2str.

The angl2str.m currently in the mapping package doesn't need round.m either but apart from format & wrapping to [-180 180] its output seems to at least "numerically" match Matlab's exactly.

Philip Nienhuis <philipnienhuis>
Project MemberIn charge of this item.
Thu 27 Aug 2020 02:39:46 PM UTC, comment #6: 

Hello.
Sorry to be so late.
I want to implement round in octave core, and finish angl2str.
The test cases for round are ready and I will ask for Matlab output in mail list.

Ricardo Fantin da Costa <ricardofantin>
Tue 25 Aug 2020 07:02:38 PM UTC, comment #5: 

Ping...

@Ricardo:
Are you still interested in helping to amend your angl2str so that it can be put in the mapping package?

Philip Nienhuis <philipnienhuis>
Project MemberIn charge of this item.
Thu 16 Jul 2020 09:45:36 AM UTC, comment #4: 

@Ricardo:
Now that I have temporary access to a real computer instead of a cell phone (I'm away from home) I could have a better look.

Well, an impressive piece of work. I wish I had known earlier, could have saved me quite some time :-)
You supplied a large amount of BIST tests as well, that's very much appreciated.

The version currently in mapping returns cell arrays (with a reason BTW), while Matlab returns char arrays (I think a sort of legacy issue there, it's a very old function).  Your version conforms to Matlab, which is preferable but does not match what I needed angl2str for. But that's an easy fix.
Matlab also doesn't wrap degree values into the 0-360 (or -180 - 180 NS) range (similarly for radians); a bit surprising IMO. ML help says "The purpose ... suitable for map display" but I have never seen maps with degree values outside those intervals (I'm a hydrogeologist and have seen quite a few maps in my almost 40 years career).
These things + the Latex output could be catched using extra Octave-specific options (along the lines of "compat" or even "traditional" or "braindead").

Let's discuss in early August how to proceed further.

Did you also start work on a str2angle.m version? (I mixed up the two in my previous comment.)

Philip Nienhuis <philipnienhuis>
Project MemberIn charge of this item.
Tue 14 Jul 2020 09:12:19 PM UTC, comment #3: 

(mapping pkg maintainer here)
Thanks for your contribution.

Indeed I recently added the angl2str and str2angle functions, primarily as some prerequisite for projection functions.
AFAICS they're Matlab-compatible, except for a few rarely used packed formats that were hard to parse using regexps. If you have a good solution for those I'm definitely interested.

I'm a bit puzzled by your remarks about the round() function. If you think that needs amendments you should open a separate bug report for that, but as round() is a core function you'd better have convincing argumentation.
Adding another round.m function to the mapping package meant to shadow the core Octave one to support just one or two individual functions might going to be a hard sell as well. There are probably several other functions in the mapping package or in its dependencies or dependers that rely on the current round() behavior; let alone core Octave functions.
There are workarounds though. More on that later on:

BTW I have very little time for Octave until the end of July so no hurry as far as I'm concerned. I'll probably look into this early August.

Philip Nienhuis <philipnienhuis>
Project MemberIn charge of this item.
Sat 11 Jul 2020 11:11:12 PM UTC, comment #2: 

Mike, thank you for the fast response.

I will update round() function in octave core and after merge the two implementations.

The current version syntax is cleaner, I could benefit from it. My version is Matlab compatible, I hope octave accept it when ready.

Ricardo Fantin da Costa <ricardofantin>
Sat 11 Jul 2020 07:34:02 PM UTC, comment #1: 

The functions 'angl2str' and 'str2angl' have already been added to the mapping package repository since the latest release. If you want to help, maybe you can compare your implementation to that and see if you can merge any improvements between the two.

Mike Miller <mtmiller>
Project Administrator
Sat 11 Jul 2020 07:22:58 PM UTC, original submission:  

Hello,
the code was tested allot. With inline functions I would change some parts, but today it is the best code that I can imagine.

Some additional parameters from round() are needed. The round.m provided should be insert in the same directory before run angl2str. I'll, eventually, update round() function from octave core.

I used a personal repository https://github.com/ricardofantin/octave to manage its versions. Some comments and the test generator to run in MATLAB are available there.

Matlab uses ^{\circ} latex output to represent degrees, Octave uses °. Besides it, the outputs are, as far as I know, exactly the same.

Ricardo.

Ricardo Fantin da Costa <ricardofantin>

 

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

Attach Files:
   
   
Comment:
   

Attached Files
file #49738:  roundn.m added by philipnienhuis (2KiB - application/octet-stream)
file #49739:  angl2str.m added by philipnienhuis (38KiB - application/octet-stream)
file #49734:  angl2str.m added by ricardofantin (38KiB - text/x-objcsrc - Current version september second 2020)
file #49471:  angl2str.m added by ricardofantin (38KiB - text/x-objcsrc)
file #49472:  round.m added by ricardofantin (877B - text/x-objcsrc)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by philipnienhuis
  • -email is unavailable- added by ricardofantin (Submitted the item)
  • -email is unavailable- added by ricardofantin
  •  

    Do you think this task is very important?
    If so, you can add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

     

     

    Follow 12 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2020-09-07 philipnienhuis StatusIn Progress => Done
        Open/ClosedOpen => Closed
    2020-09-03 philipnienhuis Attached File- => Added roundn.m, #49738
        Attached File- => Added angl2str.m, #49739
    2020-09-02 ricardofantin Attached File- => Added angl2str.m, #49734
    2020-08-30 philipnienhuis StatusNeed Info => In Progress
    2020-08-29 philipnienhuis StatusNone => Need Info
        Assigned toNone => philipnienhuis
    2020-07-14 philipnienhuis Carbon-Copy- => Added philipnienhuis
    2020-07-11 ricardofantin Attached File- => Added angl2str.m, #49471
        Attached File- => Added round.m, #49472
        Carbon-Copy- => Added ricardofantin

    Back to the top


    Powered by Savane 3.5