search

PaulWessel / CodingTasks

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

Long options batch #3 (begin 8/12/22) #3

Open rbdavis opened 2 years ago

rbdavis 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.

Will do.

rbdavis commented 2 years ago

I keep running into recurring -G confusion with these modules (now working on movie). There's often a -G spec'd on the manual page with options that appear perhaps specific to the particular module, but then there's always a link right there redirecting to a general options section 3.16 document which lists other directives and modifiers and it's not clear what applies and what doesn't. I often just wind up not including -G longoptions at all for the module. Any advice on this, either generally or in specific reference to movie?

PaulWessel commented 2 years ago

The -G is generally used for specifying a fill (color, grayscale, pattern, etc). However, it is not a common GMT option so it may vary between modules, and for many modules -G specifies output grid file, for instance. The general option link you refer to probably takes you to the page that describes the fill syntax, and that is OK. As for movie, I see it has a modifier to also set a pen for the canvas. So for the longoptions you should just treat it as any other module-specific option and here it only has a single modifier.

rbdavis commented 2 years ago

OK, will do. psxy (just submitted for PR) had the same issue. Should I implement a -G longopt there that supports only the modifiers which are explicitly shown on the psxy man page? Essentially this means ignoring the 'More' link that frequently appears on these man pages in connection with -G. I would be more than happy to make that an established policy for this ongoing process if you approve. ;->

PaulWessel commented 2 years ago

Hm, now that I am looking at the psxy man age and the (more) stuff for -G. Since this is a local option it does inherit all those modifiers that are related to the fill, i.e., the +b, +f, +r since a user may specify a pattern. Then there is the extra +z modifier as well. So the structure need all those. Since this is repeated by many modules, perhaps we need a constant like we use for GMT_INCREMENT_KW for -I? something we could extend (so that thing would have the +b+f+r but then we add +z. Perhaps not possible in a macro constant...

PaulWessel commented 2 years ago

BTW, I will look at the PR tomorrow- heading home for bed time.

rbdavis commented 2 years ago

OK, happy sleeps! I guess we'll punt on -G here for a bit. I'm having to deal with power outages here at the house yesterday/today. We're having more PV put in. Power going down now for an hour, won't be back til you're asleep.

rbdavis commented 2 years ago

Since this is repeated by many modules, perhaps we need a constant like we use for GMT_INCREMENT_KW for -I? something we could extend (so that thing would have the +b+f+r but then we add +z. Perhaps not possible in a macro constant...

Never been much of a macro wizard, partly because I get easily confused by overly complicated ones. I suppose if that's the only way to solve a problem then that's what you do.

Some followup questions/thoughts on -G, though. Is there some wad of common code somewhere that handles any more general -G option parsing that a particular module does not understand? Or does each module do all of its own self-contained -G parsing?

I am wondering if it could be useful to have two -G translation entries: one to handle modifiers or directives that are peculiar to a module and another to handle generic, more universally supported -G stuff. Here, one entry would not override the other, but rather you would have pointers to both, and when something could not be found in the module-specific entry you would then look at the other (more general) entry. That other entry might be in the common table, or perhaps even in some other special structure.

I suppose the relative usefulness of these different approaches is dependent upon whether or not there are other option types that are used in this way, how complicated is the parsing of the generalized version of each such option type, etc.

rbdavis commented 2 years ago

Can't think of good longopts names for the six (a-f) progress indicators in movie which could potentially be treated as directives. LMK if you can, otherwise I will not list directives here (or I could just call them a,b,c,d,e,f,g).

Also, movie test scripts are using a +s modifier for -P (progress) option. This is not documented by the man page. Longopts string for +s should be what?

PaulWessel commented 2 years ago

Thanks for catching the +s. I will add that to the docs as a separate PR once I finish breakfast and head into the office (need to check the code to understand again what it does). Once that is merged you will need to merge master into your branch -will let you know.

As for progress-bar names? Maybe

a: pie
b: wheel
c: arrow
d: line
e: gauge
f: axis
rbdavis commented 2 years ago

Finished longoptioning and testing movie and psevents.

Now working on grdinterpolate, where it looks like we might have a problem with a potential reverse translation of -Fl|a|c|n[+1|2] . +1 and +2 are modifiers indicating first/second derivative, but violate the rule we posited disallowing digits after a + as a legal modifier. This should not be a problem for forward translation, e.g., changing +fderiv to +1 . Will continue with grdinterpolate and psmeca on Monday.

Any further thoughts on the -G issues? -G longoptions remain unimplemented in at least a couple of the modules I've 'finished'.

rbdavis commented 2 years ago

grdinterpolate has an option -T[min/max/]inc[+i|n] |-Tfile|list . The man page does not explicitly document +i or +n , but rather refers to another paragraph Generate 1D Array which shows and explains +i and +n but using grdmath rather than grdinterpolate. However, other grdmath modifiers ( +b, +l, etc.) are also shown there, so it's not clear if those are supported by grdinterpolate as well. ???

rbdavis commented 2 years ago

Also looks like the -Z in grdinterpolate might use the same modifiers (+i, +n, +???) as the -T option. Is this true?

PaulWessel commented 2 years ago

OK, I will work on a few things tomorrow (I hope). Last week here so busy.

  1. I will fix -F to take +d1|2 instead of +1 and +2 for the reasons we discussed. Old syntax will be backwards compatible but the longoption stuff will generate +d from the modifier derivative=first|second.
  2. The -T option is used in many places so I documented it once in the Generate_1D_Array, But not all modifiers apply in all places so this needs some improvements. I will have a look but it should not affect the longoption stuff.
  3. Yes, -Z uses the same _gmt_parsearray as -T so it is very similar.
rbdavis commented 2 years ago

Thanks, Paul. So grdinterpolate's -T offers only +i and +n , and none of the other modifiers documented in that paragraph?

PaulWessel commented 2 years ago

Yes, I believe so. If not we can fix later but it seems that way.

rbdavis commented 2 years ago

Having some major and perhaps dangerous confusion on github.

Saw your e-mail earlier this morning about merging the last PR. After I did that I re-pulled the master branch to my system and it seems to have downloaded old code, e.g., psxy.c is now as it was before I added longopts support to it. Yikes?!? To figure out what might be going on I tried looking at the Issues and PRs on github but am no longer seeing anything there that I logged in relation to this longopts work?

Did I do something wrong?

PaulWessel commented 2 years ago

Hm, hard to tell (I am not much of a git guru). But I do know that master is up to date. I am on master on my laptop and psxy has your longoption stuff. If you switch to master in GitHub Desktop it may show you have lots of commits to download. Or on command line do git fetch and git pull? I merged in master to your PR before I merged the PR back into master so it is all up to date.

PaulWessel commented 2 years ago

The stuping +1 +2 in grdinterpolate is a mistake. In sample1d I have +d1 and +d2 and perhaps I just forgot. SO will fix grdinterpolate only.

PaulWessel commented 2 years ago

Submitted a PR to deal with +1|2. Now bedtime for Bonzo.

rbdavis commented 2 years ago

git mystery solved, I think. The pull I did earlier today aborted part way through but the error message made it appear that only one file download was skipped. Looks like several were skipped. After just now stashing the last three days' mods somewhere else and deleting them from src, the pull ran to completion and correctly updated psxy.c.

rbdavis commented 2 years ago

psmeca is yet another module with the same -G questions, which apply to both its -E and -G options. Omitting for now until you tell me otherwise.

rbdavis commented 2 years ago

Finished grdinterpolate and psmeca. grdinterpolate testing complete, but ctest -R seis does not appear to actually run any of the psmeca commands in test/seis/*.sh . ???

PaulWessel commented 2 years ago

Hm, maybe a cmake setting. I have

ConfigUser.cmake:#set (BUILD_SUPPLEMENTS FALSE)

but I think that is the default. Check to see. Also, see if supplements.so is built and can be found in the lib dir.

rbdavis commented 2 years ago

Hi Paul, my cmake settings same as yours. And

>  find . -name '*upplem*so' -print
./build/src/plugins/supplements.so
./vbuild/src/plugins/supplements.so.dSYM/Contents/Resources/DWARF/supplements.so
./vbuild/src/plugins/supplements.so

I can manually run all the scripts in test/seis, e.g.

>  ./full_moment_tensor.sh
gmt [WARNING]: Reformatted options: fullmt_ipts1_iref1 -J -R -Sm0.45i/8p -L0.5p,0/0/0 -G255/0/0 -N -K -O
gmt [WARNING]: Reformatted options: fullmt_ipts1_iref2 -J -R -Sm0.45i/8p -L0.5p,0/0/0 -G255/0/0 -N -K -O
gmt [WARNING]: Reformatted options: fullmt_ipts1_iref3 -J -R -Sm0.45i/8p -L0.5p,0/0/0 -G255/0/0 -N -K -O ...

and the longopts translations look reasonable, but of course this is not the full-acid test that rigorously compares GMT output product.

Just noticed something else: $DISTTOP/test has a seis subdirectory (whose scripts I mangled) but $DISTTOP/build/test does NOT have a seis subdirectory. Looks like a clue to me, but not sure what is going on behind the curtain. Running ctest -R seis is doing something, but it's NOT running the scripts in $DISTTOP/test/seis:

>  cd $DISTTOP/build
>  ctest -R seis
Test project /Users/rbd/gmt/p10repo/gmt/build
    Start 152: doc/scripts/GMT_seislegend.sh
1/2 Test #152: doc/scripts/GMT_seislegend.sh ....   Passed    0.46 sec
    Start 783: test/psscale/seis.sh
2/2 Test #783: test/psscale/seis.sh .............   Passed    0.61 sec

100% tests passed, 0 tests failed out of 2

Total Test time (real) =   1.10 sec

Instead, it's poking at stuff in doc/scripts and test/psscale . ????

rbdavis commented 2 years ago

Will longopts-mangle doc/scripts/GMT_seislegend.sh and test/psscale/seis.sh in the meantime ...

PaulWessel commented 2 years ago

Sure. And running gmt psmeca give you the usage message, meaning the supplements are built and installed just fine?

rbdavis commented 2 years ago 
> gmt psmeca
gmt psmeca  [seis] 6.5.0_c723991-dirty_2022.07.05 - Plot focal mechanisms

usage: gmt psmeca [<table>] -J<args> -R<west>/<east>/<south>/<north>[+r] ...

No point in mangling GMT_seislegend.sh or test/psscale/seis.sh. Neither actually calls psmeca, they're doing other stuff instead.

PaulWessel commented 2 years ago

I guess go through the ConfigUserAdvanced settings, e.g.

# Uncomment the following line to enable running tests of the GMT supplements
set (DO_SUPPLEMENT_TESTS ON)

and check if everything related to testing and supplements are on.

rbdavis commented 2 years ago

There look to be three other modules in seis: coupe, polar and sac. Guess I might as well handle these while I'm in the neighborhood?

Near-term work plan reminder: I've got GMT tasking pencilled in through next Wed 8/24 (including working ahead two extra days on this week's Friday holiday and Saturday), then switching back to cruise prep next Thursday. Will not work on GMT again until return from vacation on 9/26. If you have any thoughts on stuff for me to do for the next week please advise, otherwise I'll keep ploughing ahead on this stuff.

rbdavis commented 2 years ago

DO_SUPPLEMENT_TESTS was commented out. I uncommented and am doing a full rebuild, will let you know what happens. It's a bit weird that with that commented out 'ctest -R seis' still did something, but the wrong thing (maybe?).

PaulWessel commented 2 years ago

Yes, there probably are no real checks in the Cmake stuff to not allow a target like that.

Given the time schedule I do not think we should start a new venture just before you switch gears so lets stay the course. Good idea to do the entire seis supplement since there are lots of overlap, at least between coupe and meca.

rbdavis commented 2 years ago

Will do. If you have any suggestions for the next 5 to 10 modules after the seis stuff let me know.

rbdavis commented 2 years ago

Changing DO_SUPPLEMENT_TESTS and doing a full cmake rebuild from scratch did enable the test/seis tests. (The doc/scripts and test/psscale tests were also still run. I'm definitely curious as to where all this magic is happening.)

rbdavis commented 2 years ago

ENTERING MINEFIELD, TURN BACK NOW!!

After getting the real seis tests to run, all turned out fine excepting seis_11.sh which is segfaulting. This script calls psmeca indirectly, passing a bunch of psmeca command-args (which I longopt-mangled) to psevents inside a -Z command string. psevents calls psmeca with these args, and that would be just fine, except that psevents is a nosy little piece of work who wants to employ special knowledge of psmeca command args, namely (i) it wants to see a -S and (ii) it wants to NOT see a bunch of other psmeca args. I found a bunch of code in the psevents -Z processing block and adjusted it to accept psmeca longopts as well as shortops, but now we're coredumping elsewhere in psevents as I find additional dependencies on the psmeca shortops -- in fact, there are dependencies in psevents on shortops from a number of other modules as well, e.g., coupe, velo, etc.

This will take a bit to unravel and piece back together, will continue tomorrow. I am a bit concerned we may find this inter-module option co-dependency elsewhere, so if you've got any heads-ups to offer on that point that would be good to know.

PaulWessel commented 2 years ago

Yep, but I think that is pretty much the only place. I will have to fix that obviously so that events understand the long-option version of those meca/coupe commands.

rbdavis commented 2 years ago

No worries, I'm working on it today. psevents is actually in this batch of longopts updates anyway, and working on the seis modules as well makes it all the more convenient to just get this all done at the same time.

rbdavis commented 2 years ago

Hi Paul, things were going swimmingly for a while this morning with updating the option dependencies within psevents.c, then I got bogged down in testing, struggling for quite a while with a problem that may or may not exist with the test seis/seis_12.sh. I say 'may not' because I finally threw up my hands and put the old psevents.c (pre-longopts) back in place. I ran the test again and the same seis_12.sh still fails. I can't tell for sure from what I see in the testing directories but it looks like maybe there are a couple of missing beach balls from the new seis12 plot. What's doubly weird is that seis_12.sh is testing pscoupe (not psmeca) and I have not yet even touched pscoupe.

Would you mind running the seis tests on your own system and letting me know if the seis12 test passes or fails? I tried looking at output using an added -Vd within the test but nothing is jumping out at me as obviously wrong.

In the meantime I will move on to pscoupe, but will probably have no means to test it until we can figure out what's wrong with the existing seis_12.sh test (or what's wrong with me, which may well be more likely).

rbdavis commented 2 years ago

In the meantime I will move on to pscoupe, but will probably have no means to test it until we can figure out what's wrong with the existing seis_12.sh test (or what's wrong with me, which may well be more likely).

Finished working on pscoupe aside from the seis_12 test failure. Actually, besides seis_12.sh (the problematic test where pscoupe is called indirectly by psevents and the test fails), pscoupe is also called (directly) in seis_0[345].sh. These tests all pass (both pre- and post-longopts-mangling), so the mystery of the seis_12 failure deepens.

PaulWessel commented 2 years ago

Hi Roger, I will have a look. What is the name of the branch you are working on and did you publish it so it is available to me?

PaulWessel commented 2 years ago

Sorry, got side-tracked with today's community meeting. Now up on the GMT YouTube channel. You might find it interesting. As for the long-option work, this was discussed and a few useful comments emerged.

  1. It is best if you only do one module per PR, otherwise it is (a) too much for anyone peripheral to review and (b) it locks up several modules, takes longer, and makes it harder to undo things that pertains just to one module (the whole commit has to be dealt with. So from the next one, please do a single module per branch,.
  2. The perennial issue of how to assemble keywords consisting of multiple words. People have different opinions on this. The standard candidates are CamelCase, under_score, the-hyphenated, and runonsentence. THere is no single answer here. We ruled out hyphens as they are subtractions in wrappers. Since everything we do is temporary until we review and release we will probably have to revisit this one later. It is possible we will need to worry about aliases (and I think I mentioned a possibly syntax for the table being directive1|directive2|directive3, etc. and we would have to check all of them.
  3. THe issue of abbreviation came up. E.g., should --reg be allowed or must I do --region? We cannot fully answer that until all the options are coded, and then it seems we would need to flag how many letters are unique. Maybe that could be automated by real-time function that determines it on the fly. Then, perhaps --reg=world is OK but --regbad=world will not be since we will compare the users option to the start of the full option name but at least the required n leading characters, etc etc.
  4. Joaquim disagrees with some of your keywords, and I told him we do not worry too much at this now as it could change later. But he has a good point that nobody will be willing to invest lots of time to review 150 such tables and that perhaps doing one by one carefully is better if we want feedback from others. I figured we would do it later (I have not seriously reviewed too many of your choices) but it would build up.
  5. Remko wondered about allowing the Unix getops standard of having a space between the option and the argument (e.g., sort -k 4 file). I think this is problematic and breaks GMT tradition. E.g., would we allow --region world instead of --region=world? Given these are two separate argv[*] words it would be hard to know when to concatenate or not, so I would not want to go there. It would be doable if all GMT options were guaranteed to take an argument, but they do not.

I think that was the most of it.

rbdavis commented 2 years ago

Hi Roger, I will have a look. What is the name of the branch you are working on and did you publish it so it is available to me?

I just need to know if this seis_12.sh test actually passed before I started working on psevents, so testing on the master branch is perfect.

PaulWessel commented 2 years ago

Yes, seis_12.sh passes fine for me. does the log say anything specific, i.e. the rbuild/Testing/Temporary/LastTest.log file?

rbdavis commented 2 years ago

Yes, seis_12.sh passes fine for me. does the log say anything specific, i.e. the rbuild/Testing/Temporary/LastTest.log file?

Hmm, that's too bad (for me, at least ☹️ ). The log says only that the RMS is too high. And as I said, when I poked around the build/test directory looking at output products it appeared that maybe the beachballs weren't drawn. I'll run it again in a little while with -Vd and send you that output. I already looked at it myself but didn't see anything obvious. What's very weird is that when I put back the original psevents.c the test still fails. And I had not even touched pscoupe at that time.

PaulWessel commented 2 years ago

You're on an M1 mac, right? I guess I would need to look at the diff png to see if what might be going on. Perhaps you could zip up the failed test/seis/seis_12 directory and email to me.

rbdavis commented 2 years ago
  1. It is best if you only do one module per PR, otherwise it is (a) too much for anyone peripheral to review and (b) it locks up several modules, takes longer, and makes it harder to undo things that pertains just to one module (the whole commit has to be dealt with. So from the next one, please do a single module per branch,.

Sounds perfectly reasonable, will do. That may put a bit more pressure on you to turn the PRs around more quickly if anything I do involves the more commonized areas of code, e.g., the common .h file or gmt_init, etc. I think changes there become less and less frequent as we proceed, though. Obviously there are cases of it making sense to do two closely-related modules together, e.g., the psevents and seis stuff I'm doing right now.

  1. The perennial issue of how to assemble keywords consisting of multiple words. People have different opinions on this. The standard candidates are CamelCase, under_score, the-hyphenated, and runonsentence. THere is no single answer here.

I have a strong preference for camelCase as it uses minimal characters, is always intelligible, and is minimally ugly. Hyphens are a complete non-starter (oops, nonstarter) for obvious reasons. I guess I've been unconsciously using runonsentence to this point, although they're hardly sentences. Having worked through a number of modules now I think I'd prefer consistent use of camelCase (no initial cap because a large majority of these strings will be only a single word, initial capitalization thus unnecessary and distracting). I actually think we should have a single answer here unless there's a compelling reason to make exceptions, in which cases we should make those exceptions.

  1. THe issue of abbreviation came up. E.g., should --reg be allowed or must I do --region? ...

This might not be hard to do, I'd need to look in more detail. Uniqueness is only a concern within the particular module in use (and the common opts, of course). I'd say for now maybe those who like to abbreviate should just use the short-options that are already available? Which doesn't mean that we need to always code full words for every long-option string, just full enough to be understandable. And perhaps for ubiquitous options like R and J we really should just use --reg and --proj, etc., as having shorter command lines also improves readability.

  1. Joaquim disagrees with some of your keywords, and I told him we do not worry too much at this now as it could change later. But he has a good point that nobody will be willing to invest lots of time to review 150 such tables and that perhaps doing one by one carefully is better if we want feedback from others. I figured we would do it later (I have not seriously reviewed too many of your choices) but it would build up.

I'm not at all surprised that there would be many disagreements with my string choices, either stylistically or with regard to GMT-specific details (particularly the latter given my unfamiliarity with most of these modules). I think there is definitely value in having a consistent usage throughout the entire package, which does sort of assume there would be a major going-over of things at the end of the process. That effort would certainly be made easier by having people who are interested in this commenting as early in the process as possible (i.e., in the per-module PR review) so that I can take those comments into account when working on succeeding modules as well as the one in question.

  1. Remko wondered about allowing the Unix getops standard of having a space between the option and the argument (e.g., sort -k 4 file). I think this is problematic and breaks GMT tradition. E.g., would we allow --region world instead of --region=world? Given these are two separate argv[*] words it would be hard to know when to concatenate or not, so I would not want to go there. It would be doable if all GMT options were guaranteed to take an argument, but they do not.

Strongly agreed (with you). Too much work for not enough useful return, IMO.

rbdavis commented 2 years ago

s12.tar.gz

Hi Paul, I hope you can see the compressed tarfile I just pasted into this github discussion window. This is the testing output for seis_12. I followed symlinks with 'tar cvfhz' so you should be seeing the actual seis_12.sh (with added -Vd options for both psevents and its pscoupe baby) and the entire logfile output including all the -Vd stuff. If this file paste did not work let me know and I'll e-mail. Thanks!

PS: This test was run with the original psevents.c and pscoupe.c. It failed as before.

PaulWessel commented 2 years ago

Yep, see it. So for unclear reasons, when psevents calls pscoupe there are apparently no records found (the PS file shows no plotting of symbols). My guess is that psevents, which determines which records should be plotted for this events and writes them to the temp file (here /var/folders/by/5kzy8g3j6cl5ly6qkb3rlx6w00013q/T/GMT_psevents_labels_7941.txt) I bet that file is empty. So then the question is why. I can't recall if I delete those files, but you can check after you run this script for a file with that type of name in that directory.

The only strange thing for me is that those temp directories are the ones Xcode uses. When I run ctr for checking I am not running in Xcode, just the executables built by it via btr in the shell, and in that environment the temp dir is /tmp. How are you running the tests?

rbdavis commented 2 years ago

I have never used Xcode for any GMT-related purpose. In fact, I think the only time I have ever used it on this new M1 Studio system so far is just to build for MacPorts.

I typically run tests with the sample command you gave me, e.g., in this case I used

% cd $LOCALREPOPARENTDIR/gmt/build % ctest -R seis

All of the other seis tests pass (with both new and old psevents/psmeca/pscoupe code).

I will poke around that /var/folders heap, see if that file is still there (and empty as you suspect) and maybe delete anything I find, then re-run the test.

rbdavis commented 2 years ago

Just looked in /var/folders/*13q/T. GMT_psevents_labels_7941.txt is no longer there.

I doubt these are just Xcode directories. I see many other things in that particular ...*/T subdirectory, almost all of which are empty subdirectories with names of the form com.apple.whatever , e.g., com.apple.system.airplay . Just some obtuse macOS implementation of where to stuff temporary files or something?

PaulWessel commented 2 years ago

You are right - I see I have setenv TMPDIR /tmp in my .bashrc, probably to avoid having those long awkward names when hunting for stuff in temp!