Update to isd_generate.py to allow for a NAIF radius override

[thareUSGS]

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 (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

[thareUSGS]

@rbeyer You might want to have a look at this also.

[jlaura]

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.

[jessemapel]

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.

[thareUSGS]

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.

[jessemapel]

This needs tests.

[jessemapel]

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.

[jessemapel]

Fixes #480

[codecov-commenter]

Codecov Report

Merging #486 (f29fe97) into master (4f3b906) will decrease coverage by 0.11%.
The diff coverage is 0.00%.

@@            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     

Impacted Files	Coverage Δ
ale/isd_generate.py	0.00% <0.00%> (ø)
ale/drivers/chandrayaan_drivers.py	0.00% <0.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[rbeyer]

Man, I hesitate to pile more on here, but while required='--semiminor' in sys.argv is elegant, if I'm reading it correctly, it is doing the "opposite" thing the help= text for --semiminor and --semimajor are saying, since --semiminor has no required= parameter.

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:

• -a, --semimajor
• -b, --semiminor
• -r, --radius

Then write some logic after parser.parse_args() that does the following:

• If a user gives -a and not -b (or vice versa): error.
• If a user gives -r and also -a or -b: error.

Then assemble the sequence of axes to give to file_to_isd() as appropriate depending on what you got.

This allows someone to give identical values to -a and -b, or use -r as a shortcut.

Also, the help= parameters use the term ellipse but we're talking about an ellisoid here, right?

[thareUSGS]

I tested required='--semiminor' in sys.argv and I placed it exactly as you stated, and it works the opposite. completely confusing but the current code tests correctly (must have semimajor if semiminor sent).

I like the -a, -b, -r options (ala PROJ). And yes ellipsoid - I will update.

Now to figure out how to add some tests.

[jessemapel]

The existing tests for this script are here: https://github.com/USGS-Astrogeology/ale/blob/master/tests/pytests/test_isd_generate.py

[jessemapel]

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.