DOI-USGS / ale

Abstraction Layer for Ephemerides (ALE)
Other
13 stars 33 forks source link

Adds IsisDataIsisLabel driver for Kaguya #501

Closed jlaura closed 2 years ago

jlaura commented 2 years ago

Adds a KaguyaTC IsisData IsisLabel driver that supports TC1 and TC2 data.

As indicated in #493, I have some questions about how best to handle the need to furnish kernels inorder to get the distortion coefficients. I looked at the MDIS driver thinking it might furnish kernels, but I did not see how the IsisData IsisLabel was furnishing the IKs.

Fixes #493.

Marked as draft because this needs tests. I'd appreciate a review on the implementation to know if I need to furnish kernels in a different way.

jlaura commented 2 years ago

@acpaquette All comments addressed, the distortion model has been pulled out into it's own mix-in, and tests have been written. I have one commented out section in the test for the swath_mode. It does not appear that one can update the label inside the test in order to check for variations in swath mode. What is the method that has been being used to test code blocks like that?

acpaquette commented 2 years ago

@jlaura Best way to handle something like that is to pull it into its own property on the concrete driver. Something like:

@property
def swath_mode(self):
    return swath_mode

It's a little janky but it will allow you to mock that element and return whatever value you want from the label.

Another option would be to replace the last bracket accesser with a get call. Then you could mock the get call to return whatever.

Both options aren't ideal and inject some weird code smell but either will accomplish what you are looking to test

jlaura commented 2 years ago

@acpaquette Updated. I went with a _swath_mode property that is easily mocked and is then consumed by the detector_start_sample.

jlaura commented 2 years ago

@acpaquette Good to go whenever! Thanks.

jessemapel commented 2 years ago

This driver needs to have a "fast fail" instrument ID look up method so that it doesn't go all the way into the driver logic in the try-catch loop inside load.

jlaura commented 2 years ago

@jessemapel I assume this has to fail out if not 'TC1' or 'TC2'? I see the Messenger ISIS/ISIS driver using a lookup and the TGO-CaSSIS driver using a lookup implemented slightly differently. Would something like the following work?

INSTRUMENT_LIST = ['TC1', 'TC2']
@property
def instrument_id(self):
    iid = self.label['IsisCube']['Instrument']['InstrumentId']
    if iid not in INSTRUMENT_LIST:
        raise ValueError(f'{iid} not in {INSTRUMENT_LIST})
    return iid
jessemapel commented 2 years ago

Yeah, we use the instrument_id property as a first check if a driver is valid so that we can quickly iterate through them in load.

jlaura commented 2 years ago

@jessemapel Tagged you on #502 for a review. Added the fast fail and tests.