Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable slpmap to accept cubes made with elevation instead of only radius information. #3562

Closed
victoronline opened this issue Nov 26, 2019 · 11 comments
Assignees
Labels
Missions Issues which are a priority for missions

Comments

@victoronline
Copy link
Contributor

Description
Currently, DTMs must use cubes made with radius information for slpmap's height calculations. Slpmap gives an error message if the cube was made using elevation as height instead of radius.

The feature being requested is to add the option to accept cubes made with elevation information rather than only cubes with radius information, and prompt the user for the body radius to be added to the elevation data so slpmap can function normally.

Example
Slpmap would process cubes created with radius information as it normally does, but if a cube with elevation information is used, instead of producing an error, the user would be prompted for the body radius in meters which would be added to the elevation to get the proper height. This would then be used in normal slpmap calculations.

@jessemapel
Copy link
Contributor

This sounds like a reasonable enhancement and wouldn't take long. Should it accept a single radius or full tri-axial radii?

@victoronline
Copy link
Contributor Author

We only need it to be a single radius. As for whether it "should" be full tri-axial radii, I'm not sure. Jesse, I have the code ready to go with the exception of the testing.

@jessemapel
Copy link
Contributor

@victoronline We've got a mission support sprint coming up in two weeks. If you can put up a PR and send us a test and test data we can get this reviewed by then. If it's not a large change, we can probably get this reviewed before then.

@jlaura jlaura added the Missions Issues which are a priority for missions label Dec 9, 2019
@victoronline
Copy link
Contributor Author

Jesse, what would the deadline be to get this in for the sprint?

@jessemapel
Copy link
Contributor

@victoronline Whenever you get the PR in we will review it and start working with you. If you get it in this week we'll just be a bit more responsive.

To get your changes out in the next release, it needs to be merged prior to January. I don't think we've picked an exact date to freeze the code for the release, but I'd image it would be the 6th or the 13th.

@thareUSGS
Copy link

thareUSGS commented Dec 17, 2019

@victoronline I think what you want already exists and was originally addressed by this closed ticket ~5 years ago: #1795 .

The confusion might stem from fact that the Pixres parameter defaults to "Automatic" which implies it should be smart enough to handle topography in radius or more typical height elevations. But to allow for calculations from typical height elevations (using the current map projected Cartesian system), simply set pixres=file.

potential warnings:

(1) For the Cartesian method (pixres=file) the slopes are only as good as the map projection allows. Meaning if the current map projection has large horizontal distortions then the slope will also be distorted (this will generally mean slopes are more shallow). For files in a local projection like HiRISE and LROC NAC images the distortions are pretty minimal but worth noting. See this nice abstract by Thiago S. about these errors: http://www.hou.usra.edu/meetings/lpsc2014/pdf/1338.pdf

(2) For the Latitude dependent (pixres=automatic) method I am a little concerned that slope is calculated on radius values regardless if the areoid is not removed. Currently for bodies other than Earth, I think Mars is the only concern. This shouldn't be an issue for the Moon, Mercury, Venus, etc. who don't have a defined (gravity-based calculated) areoid.

@thareUSGS
Copy link

thareUSGS commented Dec 18, 2019

@victoronline So there is a use case that is currently not supported which your runtime user defined radius would still help with. This use case would be (1) you must use the Latitude dependent method (pixres=automatic), maybe helpful for say a global file) AND (2) your input file is stored in 32bit height elevations.

Thus you may still want to submit your PR for this specific use case. But -- see the next section.

Discussion: I will say slope maps are best calculated on local (and regional) high-resolution files like LROC NAC, HiRISE, CTX, Kaguya Stereo TC, HRSC. Generating slopes on more global, low-res files gets to be less reliable (even with the Latitude dependent scaling trick). And for local/regional files, which don't have tons of horizontal distortions, I would actually recommend using the Cartesian method (pixres=file) simply because it is a more standardized method to calculate slopes. There are many ways to calculate slope, but the simple Cartesian Horn method (not the Latitude dependent ISIS3 default) is widely accepted and used across many applications (for example, the Horn method is the slope default for gdaldem https://gdal.org/programs/gdaldem.html and most GIS applications like ArcMap).

more on slope equations (Horn rates well):
Jones, K.H., 1998. A comparison of algorithms used to compute hill slope as a property of the
DEM. Computers & Geosciences 24(4), 315 – 323. pdf.
--and a little discussion on Jones' finding by Jenness.

warning -- you are entering workaround territory

So if your height elevations are stored using 16bit type, then one can easily modify the label to include the additive radius value prior to running slpmap. So say you have a DEM in 16bit integers, simply add the radius to the Base keyword by running ISIS3's editlab.
editlab from=input_lunar_16bit_ElevDEM.cub options=modkey grpname=Pixels keyword=Base value=1737400.0

Now run slpmap (pixres=automatic) which will apply the radius (as set by Base=1737400.0) to the elevation heights to get radius prior to the slope calculations.

You might think -- great -- so how come I can't simply use the same editlab trick on a 32bit file? Unfortunately, editlab will allow you to modify the label, but ISIS3 doesn't not recognize Base or Multipler in 32bit labels. bummer! For 32bit files, you would need to use an app like fx or algebra to add the radius to the current elevation and write out a new file.

@victoronline
Copy link
Contributor Author

Thanks for the information Trent. It sounds like, at very least, the interface and documentaiton need an update to give the user a bit more information as to what slope equation is being used based on options selected. The documentation does mention how to run it if elevation data is used but not how the selected options can change the slope calculation used. I think the user runtime defined radius would give the user the option to choose whichever method they feel is applicable. I'm happy to proceed with the request but maybe some further discussion is warranted as to whether we add the option or just update the interface and/or documentation.

@jlaura
Copy link
Collaborator

jlaura commented Feb 23, 2020

@victoronline Is the resolution for this ticket to make documentation improvements?

@jlaura
Copy link
Collaborator

jlaura commented Mar 23, 2020

@ladoramkershner
Copy link
Contributor

@victoronline The function for calculating slope does not change between the pixres=file and pixres=automatic (default) methods. The error with negative (elevation) values arises in the latitude scaling of the x - y- resolutions done for the pixres=automatic method. It converts the input values into spherical coordinates with an ISIS method that assumes lon, lat, radius input. We have updated the documentation to hopefully illustrate this better!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Missions Issues which are a priority for missions
Projects
None yet
Development

No branches or pull requests

6 participants