Update to isd_generate.py to allow for a NAIF radius override #486
Conversation
Based on this RFC, we want to change our Mars pipeline to use a sphere. https://astrodiscuss.usgs.gov/t/appl-rfc-use-iau-sphere-for-hirise-dtm-projects-in-socet-set/428 Currently ALE uses the Mars elliptical defaults from the NAIF kernels. This update allows the user to override the kernels and set a spherical radius for both semimajor and semiminor in the ISD. This is not only needed for Mars. For triaxial bodies, the IAU recommends a best-fit sphere should also be set for map products. References (DOI) are listed in the updated help for the current IAU values.
|
@rbeyer You might want to have a look at this also. |
| "https://doi.org/10.1007/s10569-017-9805-5 or " | ||
| "http://voparis-vespa-crs.obspm.fr:8080/web/. " | ||
| "Make sure radius value is in kilometers." | ||
| ) |
There was a problem hiding this comment.
What about nargs="+" (must pass one or more values, e.g., 3396190, 3376250 OR just 3396190) or nargs="2" (must pass two values, e.g.,3396190 3396190) here and making the radius fully overridable? When the args come, as a list, you would need some logic to determine the length and apply to semi major / semi minor axes properly.
I know that the current addition fits the Mars use case, but what if I want to use old (or new) IAU radii. It seems like it would be quite nice to be able to pass these fully dynamically.
There was a problem hiding this comment.
If we want to add the ability to pass multiple radii, then we should defer to the 1 radii case as it's the most common. I do not like forcing users to enter the same radii twice when we can just add some extra logic.
There was a problem hiding this comment.
allow user to pass --semimajor as sphere or --semimajor --semiminor for ellipse. Fighting way too long with argparse to send "only" two values (as floats), so I went with two parameters. Sort of annoying but works.
| "https://doi.org/10.1007/s10569-017-9805-5 or " | ||
| "http://voparis-vespa-crs.obspm.fr:8080/web/. " | ||
| "Make sure radius value is in kilometers." | ||
| ) |
There was a problem hiding this comment.
If we want to add the ability to pass multiple radii, then we should defer to the 1 radii case as it's the most common. I do not like forcing users to enter the same radii twice when we can just add some extra logic.
|
Fixes #480 |
Add --semimajor and --semiminor parameters. I tried nargs="+" for --radius but fought for too long with argparse to do all the checking (like checking it is float). I am fine with these options but happy to hear better ways to use argparse.
Codecov Report
@@ Coverage Diff @@
## master #486 +/- ##
==========================================
- Coverage 18.56% 18.45% -0.12%
==========================================
Files 49 50 +1
Lines 5132 5164 +32
==========================================
Hits 953 953
- Misses 4179 4211 +32
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
Man, I hesitate to pile more on here, but while I have a suggestion based on the use pattern of proj (which maybe I love a little too much). Provide these three command line arguments:
Then write some logic after
Then assemble the sequence of axes to give to This allows someone to give identical values to -a and -b, or use -r as a shortcut. Also, the |
|
I tested I like the -a, -b, -r options (ala PROJ). And yes ellipsoid - I will update. Now to figure out how to add some tests. |
There was a problem hiding this comment.
The existing tests for this script are here: https://github.com/USGS-Astrogeology/ale/blob/master/tests/pytests/test_isd_generate.py
| if args.semiminor is None: # set a sphere | ||
| radii = [args.semimajor, args.semimajor] | ||
| else: # set as ellipse | ||
| radii = [args.semimajor, args.semiminor] |
There was a problem hiding this comment.
For testing, it will be easier if you move this logic into file_to_isd and just pass in semi-major, semi-minor as whatever the interface sets them to.
add in PROJ like options for radius names ("-a", "-r", "--radius", "-b")
…#620) * Update isd_generate.py add in PROJ like options for radius names ("-a", "-r", "--radius", "-b") * Update isd_generate.py updated required statement for --semimajor to catch if only --semiminor or -b is sent. Request that radius is sent in meters and updated some typos. --------- Co-authored-by: Austin Sanders <[email protected]>
For our Mars pipeline in SOCET GXP, we still are planning to use a sphere (using the Mars' areoid, see).
Currently ALE uses the Mars elliptical defaults from the NAIF kernels (or ISIS cubes). This update allows the user to override the radius values and set a spherical radius for both semimajor and semiminor in the ISD.
This is not only needed for Mars. For triaxial bodies, the IAU recommends a best-fit sphere should also be set for map products. References (DOI) are listed in the updated help for the current IAU values.
This is attempting to address #480