doi-usgs / knoten Goto Github PK

Domain reviews are specific to Scientific Software (Refer to Scientific Software IM) and ensure the software generates output that aligns with published or otherwise well-known expected results. Domain reviews may involve:

• Comparing output with external data sets
• Comparing algorithms with published, scholarly articles about the algorithm
• Reviewing unit and integration test results

Domain reviews should be performed by a trusted scientific expert with applicable subject-matter knowledge as well as sufficient applicable programming knowledge.

All software must have an administrative security review before it is made publicly available by any method. This type of review ensures personal, private, or otherwise sensitive information is not included in the repository. Types of sensitive information include:

• Personally identifiable information (PII)
• Absolute file system paths
• Internal server host names or IP addresses
• Usernames/passwords

Administrative security reviews may be performed by any trusted person; the reviewer does not necessarily need a strong scientific or programming background. When migrating an existing project into any non-private Git repository, it is important to remember that the entire project history needs to be reviewed if that history is to be maintained after migration.

Issue Description

CTX (line scan sensor model) is behaving strangely at the top of images.

Examples

I am seeing consistent, systematic errors at the top of CTX images using the CSM. These errors are evident when using the sensor model to project from the image to the ground. In the attached images, the pink pluses (+) are a grid of points at every 1000 pixels in the image, projected to an BCBF coordinate. Note the extreme shift in the uppermost set of points in this first image (B05_011720_1832_XN_03N217W).

In the next image (B04_011377_1843_XN_04N212W), the effects are less pronounced at the top of the image. Instead, note how eight rows from the bottom, the regular grid suddenly widens. Points on the left (west) side of the image maintain good ground alignment while points on the right (east) side are incorrect.

Finally, here is an example (B01_010018_1993_XN_19N191W) where the top rows of points are incorrect and are 'wrinkled' significantly to the top-right (NE). This is in contrast to the first example where the points shift dramatically to the left.

Processing Steps

• spiceinit from=lvl0 shape=ellipsoid
• gen_csm - to create the ISD file using the gen_csm.py script described by the ASP team.
• Knoten generate_ground_point using a high resolution DEM to compute the BCBF position of the intersection via the CSM. (I can provide the notebook and DEMs if desired).

Environment

ale                       0.8.7           py310h91b1402_1    conda-forge
csm                       3.0.3.3              hc9558a2_0    conda-forge
cspice                    66                h7f98852_1015    conda-forge
isis                      6.0.0                         0    usgs-astrogeology
usgscsm                   1.6.0                h924138e_0    conda-forge

Attachments

ISDs.zip

The new pfeffernusse URL is: https://astroservices.usgs.gov/pfeffer/v1/pds/

https://github.com/USGS-Astrogeology/knoten/blob/master/knoten/csm.py#L42

This should take the response and (ideally) push the string into the ISD object. I believe that the ISD only takes a filename though, so no joy there. The func needs to take a filename as an arg and offer a default that can/will be overwritten.

I followed the instructions at

https://github.com/USGS-Astrogeology/knoten/blob/master/README.md

to install knoten, so I ran the environment.yml file. But this did not install knoten, only its dependencies. I had to do, in addition,

conda install -c usgs-astrogeology -c conda-forge knoten

to install it, which was not mentioned anywhere. Perhaps another line needs to be added to the enviornment file for knoten itself?

In order to test this library, a csm is required. Right now, the swig wrapper cooks a default CSM with a number of canned values in order to test that the API is being generated correctly. We could adopt that model here. I think that that might be the safest approach, but would require moving some of the C lang compiling items into knoten making the builds slightly more complex.

Two alternatives would be to:

1. Pull the 'fake' CSM out of swig into a stand alone repo, build that a push the artifact to somewhere publicly accessible. This might be the best approach to avoid duplication.
2. Pull the usgscsm artifact in here and instantiate sensor models from that. This requires that knoten carries around ISDs, is testing using a live model, etc. I am less enamored with this approach.

Thoughts?

I installed knoten with the command:

conda install -c usgs-astrogeology -c conda-forge knoten

in a fresh environment having no other packages. Then started the python from this very environment, and did

import knoten

So far so good.

Then I did

from knoten import vis

I got the error:

Traceback (most recent call last):
File "", line 1, in
ImportError: cannot import name 'vis'

However, this works:

from knoten import csmapi

Is there a reason why vis is missing from the packaged knoten? It appears to be in the repo, and if I pulled it manually it works (kind of, some apis are broken when it comes to plotly).

For USGS software release several MetaData files need to be added/updated:

• A code.json file needs to be created and added at the top of the repo
• The LICENSE file needs to be renamed to LICENSE.md
• a DISCLAIMER.md file needs to be created and added at the top of the repo

Functions:
generate_gcps
generate_latlon_footprint
generate_bodyfixed_footprint
generate_vrt
All have default arguments using Mars radii in their argument list.

The Dawn notebook at

https://github.com/USGS-Astrogeology/knoten/blob/master/examples/dawn_fc_isis_cmp.ipynb

needs to be updated to ISIS 6, latest ALE and USGSCSM, and then the line:

usgscsm_str = ale.loads(fileName, props={'kernels': kernels}, formatter='usgscsm', verbose=False)

needs to be replaced with:

usgscsm_str = ale.loads(fileName, props={'kernels': kernels}, formatter='ale', verbose=False)

Git has moved away from the master/slave branch naming because it's culturally insensitive. Instead, new git repos now use main as the default branch. This repo still uses the old master primary branch and we should update it to use the new default branch name.

This would be a minor disruption to work.

Open pull requests will need to be changed to pull into main instead. This should not impact contributors; it will be a one time action for maintainers.

Contributors will need to pull and create branches from main instead of master. For example, to create a new branch for work, you will run git fetch upstream && git checkout -b my_work_branch upstream/main instead of git fetch upstream && git checkout -b my_work_branch upstream/master. To ensure contributors don't accidentally create branches or pull from the now out-of-date master branch, we should delete it. This will result in the old commands erroring instead of silently continuing but not actually pulling the most recent changes.

I am having issues with the line: https://github.com/USGS-Astrogeology/knoten/blob/master/knoten/csm.py#L7

which says:

from gdal import ogr

This can be reproduced on the command line, by installing the knoten dependencies, then running python as:

~/miniconda3/envs/knoten/bin/python

and then running: from gdal import ogr

It says:

ModuleNotFoundError: No module named 'gdal'

What works is to say:

from osgeo import ogr

This is on Linux. What I have installed with miniconda in the knoten environment is:

gdal 3.2.2 py37hc5bc4e4_0 conda-forge
libgdal 3.2.2 h804b7da_0 conda-forge

When creating a knotted environment, the library has a jupyter dependency. I get that the package contains notebooks, but not all users are going to want to have an arguably bloated environment that in includes the full jupyter installation.

For many, have jupyter installed in a base env and then registering the knoten kernel is the preferred workflow. Should jupyter be removed from the environment.yml because it is not a necessary component of the library? I am happy to PR in the change and include instructions in a README for how better to handle the visualizations / interactions through multiple environments.

Can knotted be released for python 3.6 - 3.9? Does not seem like the library is doing anything that is not supported by all of those versions.

Tested with a CT image that crosses the 180 boundary.

~/autocnet/autocnet/graph/node.py in footprint(self)
    641         if res is None:
    642             boundary = generate_boundary(self.geodata.raster_size[::-1])  # yx to xy
--> 643             footprint_latlon = generate_latlon_footprint(self.camera, boundary)
    644             footprint_latlon.FlattenTo2D()
    645             footprint_latlon = footprint_latlon.ExportToWkt()

~/anaconda3/envs/autocnet/lib/python3.6/site-packages/knoten/csm.py in generate_latlon_footprint(camera, boundary, semi_major, semi_minor)
    219             slope, b = regression.slope, regression.intercept
    220             current_ring.AddPoint(0 - 180, (slope*0 + b))
--> 221             current_ring.AddPoint(*switch_point)
    222             current_ring = ring
    223             current_ring.AddPoint(360 - 180, (slope*360 + b))

~/anaconda3/envs/autocnet/lib/python3.6/site-packages/osgeo/ogr.py in AddPoint(self, *args, **kwargs)
   6218     def AddPoint(self, *args, **kwargs):
   6219         """AddPoint(Geometry self, double x, double y, double z=0)"""
-> 6220         return _ogr.Geometry_AddPoint(self, *args, **kwargs)
   6221 
   6222 

TypeError: in method 'Geometry_AddPoint', argument 2 of type 'double'

If create_csm fails to create a camera, it returns None. Is this what we want it to do, or should it fail with an error?

I am using the example at https://github.com/USGS-Astrogeology/knoten/blob/master/examples/mro_ctx_isis_cmp.ipynb

to create ISD json files. I fetched knoten and am using ISIS 4.4.0, the latest.

Things work, except for the fact that the lines:

kernels = ale.util.generate_kernels_from_cube(cub_loc, expand=True)
usgscsm_str = ale.loads(fileName, props={'kernels': kernels}, formatter="usgscsm", verbose=False)

csm_isd = os.path.splitext(cub_loc)[0] + '.json'
with open(csm_isd, 'w') as isd_file:
isd_file.write(usgscsm_str)

creates .json files that are compatible with libusgscsm.so.1.4.0 and not with libusgscsm.so.1.5.1

That, despite my conda environment having

ale 0.8.5 py37h2527ec5_1 conda-forge
csm 3.0.3.3 hc9558a2_0 conda-forge
csmapi 1.0.1 py37h99015e2_0 conda-forge
usgscsm 1.5.1 h4bd325d_0 conda-forge

Is the ale package able to create the latest CSM .json files?

Or maybe I am doing something wrong.

generate_latlon_boundary doesn't seem to work for an approach/limb image, as it appears to only project points on the boundary of the image. If these pixels don't have the target in them, I'm not sure that the result will make sense.

What do we expect to be able to run the example notebooks? I have a couple questions that I think we need to answer before these are ready for consumption.

1. Are we generating ISDs in the notebook or are we adding them to the repo?

2. How are users getting the image files. Will the notebooks download them. Do we want to add them to the repo?

I would like to add some CI that runs the notebooks to ensure that the examples are still good.