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

Spice Refactor into ALE #2370

Closed
acpaquette opened this issue Feb 15, 2019 · 15 comments
Closed

Spice Refactor into ALE #2370

acpaquette opened this issue Feb 15, 2019 · 15 comments
Assignees
Labels

Comments

@acpaquette
Copy link
Collaborator

acpaquette commented Feb 15, 2019

Issue for the discussion of the following RFC for moving SPICE out of ISIS.

@jessemapel
Copy link
Contributor

We will need to support the spiceserver for both ISIS3 and ISIS4 as they will not be compatible with each other. What's the best way to do this? The current plan is to adapt pfeffernusse to be the ISIS4 spiceserver. Can we have both up and running concurrently? How long do we plan to keep both up and running?

@lwellerastro
Copy link
Contributor

@jlaura, In regard to forward and backward compatibility you say

using ISIS4 would not be readable or usable (forward compatible) in ISIS3

this completely makes sense. It might also be worth saying images spiced under ISIS3 which contain attached blobs are readable or usable under ISIS4. Am I correct in thinking this is true?

@jessemapel
Copy link
Contributor

@lwellerastro That is correct

@twilson271828
Copy link
Contributor

twilson271828 commented Feb 19, 2019

@jessemapel I don't see why it would be a problem to have both the spice server and pfeffernusse running concurrently, and then have a gradual phase out period for the spice server that is announced ahead of time. I would recommend not keeping spice server in it's current state around for too long, though. It's wobbly. Of course I don't know what the current development state of pfeffernusse is and whether it can do all of the things are current spice server class can. If it's kind of wobbly too, a quick fix could be to put spiceit into a docker container. All spiceit is is the current spice server logic placed within a simple web server.

@jlaura
Copy link
Collaborator

jlaura commented Feb 21, 2019

@lwellerastro I updated the referenced wiki page quoting your text. Thanks!

@rfergason
Copy link

Regarding the alternative option to "Retain status quo: ISIS3 technical debt remains the same and no community tool for exploiting ISIS3’s SPICE logic outside of ISIS will exist." If this option is executed, what is the impact to CSM and using that sensor model standard for planetary data outside ISIS3 (e.g., with SOCET GXP)?

@jessemapel
Copy link
Contributor

@rfergason Using the same SPICE data for an image between SOCET GXP/CSM and ISIS remains a challenge.

We would have to write tools to convert SPICE data on ISIS cubes into our ISD format. The current tools for this:

  1. Write out to socetset keyword.lis files
  2. Are written for specific instruments
  3. Sometimes require additional perl scripts to get the correct values out

If we instead, generate an ISD from the PDS file, then we have to ensure that we are:

  1. Using the same kernels as ISIS
  2. Querying the same keywords as ISIS for values

@jessemapel
Copy link
Contributor

The other direction of SOCET GXP -> ISIS is currently under the CSM integration into ISIS. If that is successful, then we can take images from SOCET GXP (that use a CSM) and load them in ISIS.

@twilson271828
Copy link
Contributor

twilson271828 commented Mar 7, 2019

Why is their code mixing between Python and C++ happening in ale.cpp? I am referring specifically to the load function near the bottom of the file. A Python interpreter is being loaded just to process Python code inside C++ to do something which looks like it would be fairly easy to code in C++. This happened in ISIS2 when Perl/Python/Fortran etc. were all mixed together, and it made that codebase completely unmaintainable.

@jessemapel
Copy link
Contributor

@twilson271828 The C++ Python mixing is happening because we are writing the Instrument/Mission specific drivers in Python. It's much easier to write and test this sort of thing in Python.

@twilson271828
Copy link
Contributor

I see Python as useful for rapid-prototyping and data analysis. For writing drivers, I don't see what the advantage is. What's easier about Python? Is it just that many of the current team members are more familiar with Python?

@jessemapel
Copy link
Contributor

Python's introspection and duck typing make the drivers much simpler to write. On the testing side, the ability to mock basically anything means that we can test everything with 0 on disk data. @Kelvinrr Can expand more.

@twilson271828
Copy link
Contributor

Yes, I would like to hear more. Thanks Jesse.

@jessemapel
Copy link
Contributor

here's an example of how we can test without data easier in Python

https://github.com/USGS-Astrogeology/ale/blob/master/tests/pytests/test_isis_spice_drivers.py

This driver reads spiceinit'd cubes and testing it would normally require a spiceinit cube, but I was able to overwrite the PVL library's load method to return a test label and then overwrite the binary table reading code to provide binary test data.

@jessemapel jessemapel mentioned this issue Dec 11, 2019
11 tasks
@jlaura
Copy link
Collaborator

jlaura commented Jan 27, 2020

Working on cleaning the wiki. We still have an open RFC on this that should be moved to adpoted because of the merge on the associated PR. Can we also close this issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

9 participants