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.

Sign up for GitHub

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

Jump to bottom

SPICE geometry enums and IMAP state function #791

Merged
merged 7 commits into from
Sep 4, 2024
Merged

SPICE geometry enums and IMAP state function #791

merged 7 commits into from
Sep 4, 2024

Conversation

subagonsouth
Copy link
Contributor

@subagonsouth subagonsouth commented Aug 28, 2024 •
edited
Loading

Change Summary

Overview

  • Add spice infrastructure code for spice bodies and frames
  • Add geometry functions for computing IMAP spacecraft state (position, velocity)
  • Add IMAP SPK kernel from Nick's 366 day simulation
  • Add test coverage for new geometry functions

New Files

  • imap_processing/tests/spice/test_data/imap_spk_demo.bsp
    • 366 day simulated IMAP ephemeris kernel

Deleted Files

N/A

Updated Files

  • imap_processing/spice/geometry.py
    • Add spice infrastructure code for spice bodies and frames
    • Add geometry functions for computing IMAP spacecraft state (position, velocity)
  • imap_processing/tests/spice/test_data/imap_test_metakernel.template
    • Add the new SPK kernel to this test template
  • imap_processing/tests/spice/test_geometry.py
    • Add test coverage for new geometry functions

Testing

2 Unit tests added

closes #787

@subagonsouth subagonsouth added the SPICE Related to SPICE label Aug 28, 2024
@subagonsouth subagonsouth self-assigned this Aug 28, 2024
greglucas
greglucas reviewed Aug 30, 2024
View reviewed changes
imap_processing/spice/geometry.py Outdated
Comment on lines 13 to 17
class SpiceId(NamedTuple):
"""Class that represents a unique identifier in the NAIF SPICE library."""

strid: str
numid: int
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can get rid of this NamedTuple and just use your Enums directly below. Maybe use IntEnum instead. But then you'd just have SpiceBody.IMAP.name which would still return the string IMAP. (The one difference you have is SSB which I think could just be written out in the long form)

Suggested change
class SpiceId(NamedTuple):
"""Class that represents a unique identifier in the NAIF SPICE library."""
strid: str
numid: int

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only issue I see with that is that some SPICE names have spaces in them.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh interesting... and unfortunate. It would be much cleaner as a normal Enum if we could somehow manage that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see any spaces in IMAP names and don't know of any SPICE names with spaces so I will implement it with just IntEnum for now.

imap_processing/spice/geometry.py Outdated
numid: int


class SpiceBody(Enum):
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is great, I really like this style of defining these as Enums.

Do we ever run into a situation of possibly getting out of sync with the SPICE kernels? Should these be dynamically looked up from the Kernels themselves somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, good question. It would be great to parse certain kernels for this information. I'd have to think about how to approach that. For example, do we check those kernels into the repo? The likelihood that any of the spacecraft or instrument values change should be small. I can write a ticket up for this but think that for now it is a nice to have that would take too much development time.

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 Completely agree that this is very minor. I was just curious, so definitely punt on it into the very distant future.

imap_processing/spice/geometry.py Outdated Show resolved Hide resolved
imap_processing/spice/geometry.py Outdated Show resolved Hide resolved
imap_processing/tests/spice/test_geometry.py Outdated Show resolved Hide resolved
laspsandoval
laspsandoval approved these changes Sep 3, 2024
View reviewed changes
Copy link
Contributor

@laspsandoval laspsandoval left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good to me. Thanks for the addition. Just a few changes along with Greg's.

imap_processing/spice/geometry.py Show resolved Hide resolved
imap_processing/spice/geometry.py Outdated
from imap_processing.spice.kernels import ensure_spice


class SpiceId(NamedTuple):
Copy link
Contributor

Choose a reason for hiding this comment

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

Would this not work here?

https://naif.jpl.nasa.gov/pub/naif/toolkit_docs/C/cspice/bodn2c_c.html
sun_id = spice.bodn2c("SUN")

imap_processing/spice/geometry.py Outdated Show resolved Hide resolved
tech3371
tech3371 approved these changes Sep 4, 2024
View reviewed changes
Copy link
Contributor

@tech3371 tech3371 left a comment

Choose a reason for hiding this comment

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

With little I know, it looks good to me. I was able to follow it on high level.

imap_processing/spice/geometry.py Outdated Show resolved Hide resolved
@subagonsouth
Add spice infrastructure code for spice bodies and frames
6022191 
Add geometry functions for computing IMAP spacecraft state (position, velocity)
Add IMAP SPK kernel from Nick's 366 day simulation
Add test coverage for new geometry functions
@subagonsouth
Remove _ensured_spkezr funciton.
816aeee 
Remove SpiceId(NamedTuple)
Parameterize test to cover single and iterable ET input
Change typing on ensure_spice to apease mypy
Fix up ensure_spice decorator factory typing
@subagonsouth
Fix typo
f1c50eb
@subagonsouth
Use IntEnum
00d6569
@subagonsouth
Back out some of the typing changes not compatible with Python 3.9
012a20c
@subagonsouth
Use spice to define built-in bodies and frames
c93d0e5
@subagonsouth
Tell mypy not to check skip type check
55320bf
@subagonsouth subagonsouth force-pushed the 787-add-some-spice-infrastructure-and-simple-geometry-function branch from 0482da9 to 55320bf Compare September 4, 2024 22:35
@subagonsouth subagonsouth merged commit 8b7d5e8 into IMAP-Science-Operations-Center:dev Sep 4, 2024
17 checks passed
@subagonsouth subagonsouth deleted the 787-add-some-spice-infrastructure-and-simple-geometry-function branch September 4, 2024 22:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@greglucas greglucas greglucas left review comments

@tech3371 tech3371 tech3371 approved these changes

@laspsandoval laspsandoval laspsandoval approved these changes

@vmartinez-cu vmartinez-cu Awaiting requested review from vmartinez-cu

Assignees

@subagonsouth subagonsouth

Labels
SPICE Related to SPICE
Projects
IMAP
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add some SPICE infrastructure and simple geometry function
4 participants
@subagonsouth @greglucas @tech3371 @laspsandoval