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

Add Kaguyatc IsisLabel, Naifspice driver #487

Merged
merged 15 commits into from
Mar 6, 2023
Merged

Conversation

acpaquette
Copy link
Collaborator

Adds the kaguyatcIsisLabelNaifSpice driver to ale for use with ISIS. Also fixes a few issues with the kaguyami load test

@jlaura
Copy link
Collaborator

jlaura commented Oct 22, 2022

@acpaquette I pulled this PR local to test it on KaguyaTC spiceinited cubes. I am getting the following error from the new driver:

Trying <class 'ale.drivers.selene_drivers.KaguyaTcIsisLabelNaifSpiceDriver'>
Failed: Failed to find metakernels. mission: selene, year:2007, versions="latest" spice root = "/isisdata/"

Why is the KaguyaTcIsisLabelNaifSpiceDriver looking for metakernels? The kernels are already listed on the ISIS cube.

@jessemapel
Copy link
Contributor

@jlaura The drivers do not have any kernel selection logic in them. That is the job of SpiceQL. It sounds like you want the IsisLabelIsisSpice driver.

@jlaura
Copy link
Collaborator

jlaura commented Oct 22, 2022

Yup, I realize now that this driver is not going to work either. Thanks for the assist!

I do need the ISISSpice ISISLabel driver for these data.

ale/base/label_isis.py Show resolved Hide resolved
ale/base/label_isis.py Show resolved Hide resolved
ale/drivers/selene_drivers.py Show resolved Hide resolved
ale/drivers/selene_drivers.py Outdated Show resolved Hide resolved
ale/drivers/selene_drivers.py Show resolved Hide resolved
ale/drivers/selene_drivers.py Outdated Show resolved Hide resolved
ale/drivers/selene_drivers.py Outdated Show resolved Hide resolved
@jlaura
Copy link
Collaborator

jlaura commented Nov 8, 2022

@acpaquette Sorry for the delay!

@jlaura jlaura changed the base branch from master to main February 23, 2023 16:59
@acpaquette acpaquette closed this Mar 2, 2023
@acpaquette acpaquette reopened this Mar 2, 2023
jlaura
jlaura previously approved these changes Mar 3, 2023
ale/drivers/selene_drivers.py Show resolved Hide resolved
ale/drivers/selene_drivers.py Outdated Show resolved Hide resolved
ale/drivers/selene_drivers.py Outdated Show resolved Hide resolved
ale/drivers/selene_drivers.py Outdated Show resolved Hide resolved
ale/drivers/selene_drivers.py Outdated Show resolved Hide resolved
ale/drivers/selene_drivers.py Outdated Show resolved Hide resolved
ale/drivers/selene_drivers.py Outdated Show resolved Hide resolved
ale/drivers/selene_drivers.py Outdated Show resolved Hide resolved
def boresight_x(self):
"""
Returns the x focal plane coordinate of the boresight.
Expects ikid to be defined. This should be the NAIF integer ID for the
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we needs these expects? The API called doesn't need to know this information because this class has an ikid property. The code is explicit that self.ikid is needed. Maybe these 'expects` in the docstring can be removed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They were/are a way to tell people what other properties a property relies on, since many of the properties have default implementations

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that, but this is a 3 line function and the code clearly needs the ikid.

ale/drivers/selene_drivers.py Outdated Show resolved Hide resolved
@jlaura jlaura merged commit d3f02a0 into DOI-USGS:main Mar 6, 2023
jrcain-usgs pushed a commit to jrcain-usgs/ale that referenced this pull request May 19, 2023
* Made isis label spacecraft_clock_start/stop_counts handle keywords with units

* Add new kaguya isislabel naifspice driver

* Add test and test data for the new kaguya driver

* Fixes to kaguyami tests and updated kaguyatc tests

* Adds missing kernels for kaguyami test

* IsisLabelNaifSpice driver and IsisLabelIsisSpice driver now get the right exposure duration key

* Fixes for ISIS, CSM inconsistances

* Changed any sct2e call that converted the string to float to scs2e calls

* Fixed logging in isd_generate

* Fixes inconsistencies between kaguyaTC drivers

* Fixed tests

* Wrapped all spice functions in not hasattr checks

* Removed spacecraft_name override in IsisLabel NaifSpice driver

* Fixed up drivers and tests

* Fixed haiku like doc string descriptions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants