How do we want to denote USGSCSM vs ISIS as the sensor model we're generating orientations for?

jessemapel commented 5 years ago

Right now the base Driver class has lots of things hard coded into it that work for USGSCSM, but not ISIS. For example, the sensor orientation is a single set of time dependent quaternions. For ISIS we need to have time dependent quaternions and a single constant rotation matrix.

One solution I see is creating UsgsCsmDriver and IsisDriver subclasses and moving all of the specific code into those.

jlaura commented 5 years ago

Before any of this implementation happens, do we have a design doc of any sort? Rationale behind using mixins how we are, etc? That might help inform the design decision here.

Off the cuff, the above might be awesome or it might require a BetterSensoModelAPIofFooDriver in the future.

jessemapel commented 5 years ago

Here is some very basic design things I have in my head right now:

User Stories

This is the high level collection of user stories

For a range of different instruments be able to do the following:

generate a USGSCSM ISD using a PDS3 label and SPICE kernels
generate a USGSCSM ISD using a PDS4 label and SPICE kernels
generate a USGSCSM ISD using an ISIS3 Cube label and SPICE kernels
generate a USGSCSM ISD using an ISIS3 Cube label and attached SPICE data (i.e. no kernels)
generate the data currently generated by spiceinit using an ISIS3 Cube label and SPICE kernels
generate the data currently generated by spiceinit from an ISIS3 Cube label and attached SPICE data

Overall Design

I see the user stories as permutations of 4 categories:

An instrument
1. MDIS
2. CTX
3. CaSSIS
4. etc.
An end point.
1. USGSCSM ISD
2. Attached SPICE data for an ISIS sensor model
A label.
1. PDS3
2. PDS4
3. ISIS3
Some additional data source
1. SPICE kernels
2. Attached SPICE data

We want to create a driver for each permutation. In order to cut down on code duplication, we need a design that allows us to write the code for each different case of each category once and then combine them in different permutations to create our drivers.

Mix-Ins

Currently, we use mix-ins to emulate the permutations. So, for each case in each category, we write a mix-in and then the drivers are created by mixing them all together.

Interfaces and Polymorphism

We could also use interfaces and polymorphism to make this work. Each category could have an interface defined for it and then we just write implementations of that interface for each case.

Which should we use?

I think either option will work, it's just a question of what aspects we value higher. The main advantage of mix-ins is that they are unstructured and they make sharing information between the different categories easier. The main advantage of interfaces and polymorphism is that they are much easier to test in isolation and better enforce a consistent structure across the library.

Where we are right now

I feel like we've gone down the mix-in path a bit too far. Everything is too un-structured and it's not clear what different mix-ins should be responsible for. The other issue is that we have too complex of inheritance going on in out mixins. We are inheriting across categories right now and we should NOT be doing that to avoid the multiple inheritance diamond issue.

jlaura commented 5 years ago

https://gist.github.com/jlaura/75bd45496f4bdcbee227a84095769557

I got to thinking about this on a run and here is another idea. I do not think that the above categories are as cleanly delineated as we might hope. Take a look at Kaguya for example. I also struggled to think of a clean way to use either multiple inheritance or ABCs in a way that would allow for a more generalized (not instrument specific) solution. This got me thinking about how to go about defining an interface then, while trying to maintain some semblance of extensibility.

The linked gist takes the approach where everything exposes some dict-like interface, the inheritance tree is flat, and the difference between a label, ISIS, spice, etc. are hidden inside of the associated classes. I am happy to chat about this and get some hard feedback.

jessemapel commented 5 years ago

@jlaura I like the flat structure of that and I really like mixing things in when the driver is created. Here's some questions/problems I see:

How does the code know what it needs to get from the driver? The point of ALE is to make it so you don't need to know exactly what you need for the sensor model, I want to be able to generate everything in just a single call.
How do you specify an output format? This is related to the previous question, the user has to know the format they are writing out and exactly how these ALE properties relate to the output format.
How would your example MDIS driver work with a non-SPICE source? It has a spice call hard coded into it.
We would need additional code to identify what type of data sources should be used.

jlaura commented 5 years ago

@jessemapel I have been thinking a bunch about this on weekend runs. Still have issues that could definitely benefit from more critical feedback / identification of the breaks, etc.

Output Formats

My thinking here is write a to_* function that knows how to perform the conversion. Since these are written once per format we have two options: (1) write a to_dict style function that uses code introspection or (2) write a 'complete' driver where the developer goes through and says: 'for ISIS3 I need X, Y, Z', so I need to write to pull keys X,Y,Z'. I am partial to the second option because the implementation only has to be done once and the code is very explicit then. This also means that the code is easier (I would suggest - I think this is debatable) to test. I might be splitting hairs here where the driver has no idea what information is needed, but the to_* functions do.

Non-spice

This still has be pseudo-stumped and was the first issue that saw when trying to get something mocked up. I think the solution is to have the specific driver (MDIS) subclass the appropriate classes. For example, the starting_detector_sample is an implementation specific entry in the IK, so we should probably have:

class MDISSpice(Spice):
    @property
    def starting_detector_sample(self):
        """
        Returns starting detector sample quired from Spice Kernels.

        Returns
        -------
        : int
          starting detector sample
        """
        return 'Foo'  # Just hard coded in the example to show a return value, in reality this would come in from the line below
        return int(spice.gdpool('INS{}_FPUBIN_START_SAMPLE'.format(self.get('ikid'), 0, 1)[0]))

I would definitely solicit all thoughts here as I am struggling to identify a clean way to do this. If we take the ISIS3 example, we should never need to subclass because the ISIS3 label is standardized (?). For PDS and Spice, I think that we will because the missions do different things. The interface would remain the same and it would be good to think about how to enforce the separation between a spice data source and a label object (I am suggesting that we break the current, implicit connection).

Additional Code

Spot on. We could (1) push this to the user :-1:, (2) write a driver iteration style method that does a bunch of composition on the fly (probably brittle though we could spend some time here), (3) write a number of explicit driver combinations that we know will work together (ISIS3, ISIS3/Spice, PDS3/Spice, etc.) and then iterate over those. In the short-short term, I might be inclined to actually use 1 and then grow into 3.

jessemapel commented 5 years ago

New architecture solves this be having to_* formatting methods that take a driver and output the appropriate format

DOI-USGS / ale