PaulWessel / CodingTasks

Organized issues tracker for tasks related to GMT
0 stars 0 forks source link

Long options batch #2 (begin 8/6/22) #2

Open rbdavis opened 2 years ago

rbdavis commented 2 years ago

Hi Paul, I haven't been able to find your latest suggestion of modules for me to work on (after pswiggle). If you can get me a list whenever that's great, I'll just pick my own until I hear from you.

PaulWessel commented 2 years ago

Right, not sure if I made a longer list. I dont have a good system for this but you've done some plotting modules and some calculation modules (block*, surface). Maybe look at one with many options and modifiers, such as mapproject, grdtrack, psxu?

rbdavis commented 2 years ago

Will do, thanks.

rbdavis commented 2 years ago

Hi Paul, you may recall that when I paused two weeks ago I had just come across a problem involving the ambiguity of + in option strings, i.e., sometimes it indicates a modifier, sometimes it's an arithmetic sign and sometimes it's part of an (archaic?) argument specification. To recap:

I said:

Paul, there was one little hiccup with today's grdimage testing: translation of -I+0.5 led to a translation error when I hand-mangled that to --intensity=+0.5 and then ran it through the translator. This is essentially the same error as the other day's -G+green, i.e., a + that was part of an argument within an option spec is mistakenly interpreted by the translator as the trigger for a modifier. Although the -G+green usage may be a rare archaism not worth the time expended to cause it to be properly handled, it seems likely to me that we may come to grief more frequently with today's example where the + is instead being used as a numeric sign. I think I should take another look at the translator to weed out cases of =+ two-character sequences from being considered as part of a modifier. The only reason we might not want to do this is if there are possible short-option specs where some argument which immediately precedes the + trigger for a real modifier actually ends in an = . That seems unlikely to me?

You said:

We should probably check that + is followed by a lower or upper-case letter since we cannot insist that nobody use a + sign in numbers. They may also enter things like -L1.5e+5 and we need to not get triggered by that either.

So, the definition of a modifier will need to be +letter.

I said:

I should be able to easily do both tests: (i) character before + cannot be = , and (ii) character after + must be a letter. That would support both the -G archaism and signed numeric strings. Does that seem reasonable?

In testing new code in the translator today I have unfortunately discovered that we cannot disqualify the two-character sequence =+ from being the beginning of a modifier. This because it's possible to have an initial --whatever longoptions flag which has no directive and is immediately followed by a modifier. For instance, in one of the pscoast tests we have -A+aiwhich in longoption form is --area=+antarctica=i. This isn't too terrible, I guess, as things like --intensity=+0.5 will still be disqualified from modifier-hood as the character after the + is non-alphabetic. Unfortunately I don't see how we're going to be able to make anything like -G+green work. Modifying the translator again to omit this =+ test and then retesting ...

rbdavis commented 2 years ago

... re-tested all longoptions modules with new translator fixes, all good (including --intensity=+0.5 and --area=+antarctica=i ) Continuing with next module tomorrow.

PaulWessel commented 2 years ago

Sounds good. But I wonder if the equal-sign after the directive is only required if there is an argument? Dong we list --longoption[=][modifiers]? So should it not be --area+antarctica=i ?

rbdavis commented 2 years ago

Yes, you're correct. I was actually having second thoughts earlier this morning on that point. Your comments in the common .h file are

--<long_option>[=<long_directives>[:=<arg>]][+<long_modifier1>[=<arg1>]] ...

I'll make sure the translator works properly without the = (e.g., for --area+antarctica=i ), then restore the two-character =+ modifier-disqualification test if so.

rbdavis commented 2 years ago

Hi Paul, mapproject man page says -F[e|f|k|M|n|u|c|i|p]but then says to refer to the Units section of the very same man page which (i) also mentions d, m and s, but (ii) does not mention c, i, or p. Are all-of-the-above unit types supported, e.g., should the man page actually say -F[d|m|s|e|f|k|M|n|u|c|i|p] ?

PaulWessel commented 2 years ago

Yes, think so

rbdavis commented 2 years ago

OK, thanks!

rbdavis commented 2 years ago

Hi Paul, I have a a question about -bi and -bo which seem similar to -di/-do. In the common .h file we now have

    {   0, 'b', "binary",
                "",                      "", 
                "b,l",                   "bigendian,littleendian" },
    ...
    {   0, 'd', "nodata",
                "i,o",                   "in,out",
                "c",                     "column" },

Any reason we should not change this -b entry to

    {   0, 'b', "binary",
                "i,o",                   "in,out", 
                "b,l",                   "bigendian,littleendian" },
rbdavis commented 2 years ago

Hi Paul, I pretty much waded all the way through mapproject today. Besides the above questions, I have a couple more here that I wanted to get off before calling it a day:

(1) You coded the translator to search for common option keyword matches before module-specific matches. Any particular reason for that, and would you consider doing it the other way around (to allow module-specific keywords to override common)? Seems to me there could be some value in doing it the latter way, although I don't have an immediate need. This came up today because I mistakenly used --distance in the mapproject-specific table, which duplicated a common-option --distance I had forgotten about. The latter matched when I began testing (being searched first by the existing code as noted), and this in turn caused a core dump, probably because of mismatched details in the translator's common options table entry. I will be running that core dump down tomorrow (as core dumps offend what is pretty much my only religious principle ;-> ), but that's kind of a side-issue in regard to which table the translator should search first -- any thoughts?

(2) I felt really out of my depth in making up longopts strings for mapproject, and I suspect you will not like much of what I made up. So, in the interest of saving time it might not be a bad idea if you want to comment on the strings I've got now before I bother with the PR. Here's the mapproject table with all longopts strings:

        /* separator, short_option, long_option,
                  short_directives,          long_directives,
                  short_modifiers,           long_modifiers */
        { 0, 'A', "azimuth", 
                  "b,B,f,F,o,O",             "back,backgeodetic,forward,forwardgeodetic,orient,orientgeodetic",
                  "v",                       "variable" },
        { 0, 'C', "center", 
                  "",                        "",
                  "m",                       "stdparallel" },
        { 0, 'D', "lengthunit",
                  "c,i,p",                   "cm,inch,point",
                  "",                        "" },
        { 0, 'E', "ecef",                    "", "", "", "" },
        { 0, 'F', "forcescale",
                  "d,m,s,e,f,k,M,n,u,c,i,p", "deg,min,sec,meter,foot,km,smile,nmile,ussft,cm,inch,point",
                  "",                        "" },
        { 0, 'G', "dystance",
                  "",                        "",
                  "a,i,u,v",                 "accumulated,incremental,unit,variable" },
        { 0, 'I', "inverse",                 "", "", "", "" },
        { 0, 'L', "multisegment",
                  "",                        "",
                  "p,u",                     "segmentpoint,unit" },
        { 0, 'N', "latconvert",
                  "a,c,g,m",                 "authalic,conformal,geocentric,meridional",
                  "",                        "" },
        { 0, 'Q', "listprojparams",
                  "d,e",                     "datums,ellipsoids",
                  "",                        "" },
        { 0, 'S', "suppress",                "", "", "", "" },
        { 0, 'Q', "transformdatum",
                  "h",                       "height",
                  "",                        "" },
        { 0, 'W', "mapinfo",
                  "e,E,g,h,j,n,o,O,r,R,w,x", "encompass,encompasstext,plotcoords,height,justify,normalize,cornercoords,regiontext,width,xy",
                  "n",                       "npoints" },
        { 0, 'Z', "traveltime",
                  "",                        "",
                  "a,i,f,t",                 "accumulated,incremental,isoformat,epochtime" },
PaulWessel commented 2 years ago

Any reason we should not change this -b entry to

No, I think they both should be similar. We do use -bi and -bo and if no i|o it applies to both input and output. So please update that.

PaulWessel commented 2 years ago

(1) You coded the translator to search for common option keyword matches before module-specific matches.

I think we always expect the common options (even in long-format) to be unique and not "overloaded" by a module. The processing in GMT always calls GMT_Parse_Common first, then the local parse function, so I think we do not want to change that order of things. It could open up unintended consequences.

PaulWessel commented 2 years ago

Here's the mapproject table with all longopts strings:

No worries, I will review these later today and hopefully you will get specific feedback by the start of your day!

PaulWessel commented 2 years ago

While all long options and modifier words may change before we release this, for now I will only adjust those I think needs to be replaced. So for mapproject my list is this:

-C: use merclat instead of stdparallel since the latter sounds broader than just a Mercator setting -F: The "force" in the docs is a bit of a misnomer I think. It is about setting projected distance unit, so perhaps --projunit? -G: I see where the distance came up. Let's try --stride for now -L: Try --proximity

rbdavis commented 2 years ago

Thanks Paul, will implement all as you suggested!

rbdavis commented 2 years ago

All of the above fixed, including core dump, and mapproject longopts tested and passed EXCEPT:

mapproject pt.txt -J+proj=moll+ellps=WGS84+units=m ...

Don't see how we can ever get -J working with +proj options due to all the + characters used, many of which are followed by alphabetic characters which allow the preceding + to pass the modifier test. I seem to recall you had sort of given up hope on -J longopts anyway, perhaps this was the reason?

rbdavis commented 2 years ago

Hi Paul, finished (sort of, see next message) with grdtrack. Here are the longopts strings for your comments:

        /* separator, short_option, long_option,
                  short_directives,          long_directives,
                  short_modifiers,           long_modifiers */
        { 0, 'A', "resample",
                  "f,p,m,r,R",               "keeporig,mfollow,pfollow,equidistant,exactfit",
                  "l",                       "rhumb" },
        { 0, 'C', "crossing",
                  "",                        "",
                  "a,v,d,f,l,r",             "alternate,wesn,deviant,fixed,left,right" },
        { 0, 'D', "linefile",                "", "", "", "" },
        { 0, 'E', "linesegs",
                  "",                        "",
                  "a,c,d,g,i,l,n,o,r",       "azimuth,connect,distance,degrees,incr,length,npoints,origin,radius" },
        { 0, 'F', "findcross",
                  "",                        "",
                  "b,n,r,z",                 "balance,negative,rms,zvalue" },
        { 0, 'G', "grid",
                  "",                        "",
                  "l",                       "list" },
        { 0, 'N', "noskip",                  "", "", "", "" },
        { 0, 'S', "singleprofile",
                  "a,m,p,l,L,u,U",           "average,median,mode,lower,lowerpos,upper,upperneg",
                  "a,d,r,s,c",               "values,deviations,residuals,save,envelope" },
        { 0, 'T', "nearestnode",
                  "",                        "",
                  "e,p",                     "report,replace" },
        { 0, 'Z', "zvalues",                 "", "", "", "" },
rbdavis commented 2 years ago

Something strange is happening with the grdtrack longopts testing, namely in the two test scripts layout.sh. When I run the shortopts versions of layout.sh the two tests pass. When I run the longopts versions of these scripts they fail with RMS errors, e.g.:

% ctest -R grdtrack --rerun-failed --output-on-failure Test project /Users/rbd/gmt/p10repo/gmt/build Start 533: test/grdtrack/layout.sh 1/2 Test #533: test/grdtrack/layout.sh ..........***Failed 1.12 sec Set GMT_SESSION_NAME = 39621 gmt [WARNING]: Reformatted options: @Matthews_2016_subduction_subset.txt -Gdummy.nc -C500k/100/100+v gmt [WARNING]: Reformatted options: @Matthews_2016_subduction_subset.txt -Gdummy.nc -C500k/100/100+a gmt [WARNING]: Reformatted options: @Matthews_2016_subduction_subset.txt -Gdummy.nc -C500k/100/100 pstext [WARNING]: Skipped 1 records as blank - please check input data. /opt/local/bin/gm compare: image difference exceeds limit (0.0311458 > 0.003). test/grdtrack/layout.ps: RMS Error = 0.0311 [FAIL] memtrack errors: 0 exit status: 1

Start 534: test/grdtrack/layout_fixed.sh

2/2 Test #534: test/grdtrack/layout_fixed.sh ....***Failed 1.15 sec Set GMT_SESSION_NAME = 39717 gmt [WARNING]: Reformatted options: @Matthews_2016_subduction_subset.txt -Gdummy.nc -C500k/100/100+f130 grdtrack [WARNING]: Some points along your profiles were outside the grid domain(s). grdtrack [WARNING]: Some input points were outside the grid domain(s). gmt [WARNING]: Reformatted options: @Matthews_2016_subduction_subset.txt -Gdummy.nc -C500k/100/100+f90 gmt [WARNING]: Reformatted options: @Matthews_2016_subduction_subset.txt -Gdummy.nc -C500k/100/100 pstext [WARNING]: Skipped 1 records as blank - please check input data. /opt/local/bin/gm compare: image difference exceeds limit (0.034924 > 0.003). test/grdtrack/layout_fixed.ps: RMS Error = 0.0349 [FAIL] memtrack errors: 0 exit status: 1

This is repeatable (both the shortopts passes and the longopts failures. Yet, the translations of the longopts commands appear to be correct, e.g.:

gmt [WARNING]: Reformatted options: @Matthews_2016_subduction_subset.txt -Gdummy.nc -C500k/100/100+v gmt [WARNING]: Reformatted options: @Matthews_2016_subduction_subset.txt -Gdummy.nc -C500k/100/100+a gmt [WARNING]: Reformatted options: @Matthews_2016_subduction_subset.txt -Gdummy.nc -C500k/100/100

I'm at a loss here as to how to proceed. Any ideas?

rbdavis commented 2 years ago

I'll be working at UHMC tomorrow on more survey prep for one day, which I will make up for you with one day during my next 2-week-ish survey prep stint. Mark needs a hand with hooking up our towfish to the computer systems and can't proceed until I help him out. I will definitely be back on GMT Thursday and following days. I'm sending you an e-mail with a more detailed work plan proposal for August/September which will hopefully give you a better idea of my plan.

PaulWessel commented 2 years ago

Add -Vd to the grdtrack command and see if you learn more about what is going on. Also check that your ~/.gmt/cache/Matthews_2016_subduction_subset.txt file is not corrupted or blank or something (it complaints there are blank records so that is fishy). Delete that file and rerun the script to see if that clears it up (It will try to download again).

rbdavis commented 2 years ago

Will try -Vd when I get back on this tomorrow. I suspect datafile is not the problem as the test passed when running the shortopts test scripts. Will delete and retry anyway, of course.

rbdavis commented 2 years ago

Mystery solved re: the grdtrack test errors, I think. The two layout*sh test scripts each create 3 plots on one page, with each plot using a different modifier (e.g., (i) no modifier, (ii) +f130, and (iii) +f90). The plots are labeled with these actual modifier strings to show how each plot was made. Because I used longopts modifier strings and the scripts are grabbing those strings from the grdtrack command lines the plot labels have therefore all changed, e.g., from +f130 to +fixed=130 . Everything else about the plots appears identical.

rbdavis commented 2 years ago

Maybe look at one with many options and modifiers, such as mapproject, grdtrack, psxu?

Hope you meant psxy? I can't find any psxu, working on psxy now.

rbdavis commented 2 years ago

Not sure what to do about psxy's -G option. The man page refers to the general -G options but also seems a bit contradictory/confusing (+z?).

rbdavis commented 2 years ago

psxy's -S looks like a complete minefield: zillions of modifiers, some meaning different things in different circumstances, plus another entire set of vector modifiers which have other meaning. Have to punt on this, I think.

Same for psxy's -W also? Too many modifiers including vector modifier set?

rbdavis commented 2 years ago

Any comments on the longopts strings I proposed a couple of days ago for grdtrack (see above)?

Here is the next batch for psxy, please comment as you see fit:

    { 0, 'A', "straightlines",       
              "m,p,x,y,r,t",             "mpfollow,pmfollow,xyalong,yxalong,rtalong,tralong",
              "",                        "" },
    { 0, 'C', "cpt",                     "", "", "", "" },
    { 0, 'D', "offset",                  "", "", "", "" },
    { 0, 'E', "errorbars",
              "x,y,X,Y",                 "xbar,ybar,boxwhisker,stemleaf",
              "a,A,c,n,w,p",             "asymmetrical,lhbounds,symbolfill,notch,capwidth,pen" },
    { 0, 'F', "scheme",
              "c,n,p",                   "continuous,network,refpoint",
              "",                        "" },
    { 0, 'H', "scale",                   "", "", "", "" },
    { 0, 'I', "intensity",               "", "", "", "" },
    { 0, 'L', "polygons",            
              "",                        "",
              "b,d,D,x,y,p",             "bounds,symdev,asymdev,xanchor,yanchor,pen" },
    { 0, 'N', "noclip",              
              "c,r",                     "clipnorepeat,repeatnoclip",
              "",                        "" },
    { 0, 'T', "ignoreinfiles",           "", "", "", "" },
    { 0, 'Z', "zvalue",                  "", "", "", "" },
rbdavis commented 2 years ago

I tested the psxy longopts strings but there are a huge number of psxy test scripts (over 150?). I successfully converted and ran about a third of them (the first third, all scripts beginning with the letters [A-Zabcd] ) before I got exhausted and gave up. Please advise if you'd like me to actually test the rest or not before I spend the time on converting the other ~100 scripts.

Whenever I finish with the psxy testing (now? ;-> ) I should probably break for a PR before moving on. After that, any suggestions for the next batch of modules to do?

PaulWessel commented 2 years ago

Yes, I think that is good enough for psxy. Maybe next on the list could be: movie, psevents, grdinterpolate? Also, we should try one of the supplememts in case there are some lessons with that, e.g., seis/psmeca.c, and perhaps some of the seismologists will have comments on our keyword selections.