-
-
Notifications
You must be signed in to change notification settings - Fork 582
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 module to use SkyCoord for SPICE computations #7237
Conversation
6d6981d
to
84b5951
Compare
this is very nice |
Given SPICE is used by a wider set of people (/ communities) than just solar physics, I think it's worth having a conversation about whether putting this in sunpy is the best way forward, compared to a separate package or in astropy core. In particular, having a separate package might encourange a wider set of contributors and eyes on the code from different research communities. (I appreciate astrospice is archived, but that doesn't stop someone forking it and continuing development) |
First off, this is awesome. astrospice also has helpers for getting kernels etc from the net iirc, which is probably useful additional functionality to go along with this? We could resurrect astrospice if we think that's the best place for this to live, but I am not sure I know enough about SPICE and how hard all this is going to be to maintain that I can make a very qualified decision. |
In the long term, a mature version of this bridging module should probably live in Astropy core. I feel that it is too few code lines and too integrated into the coordinates framework to separate out to its own package (e.g., In the short term, I think maturing this module in SunPy core is the most effective approach. Development can be agile while also automatically distributing the module to the community where the module will have the most impact, and thus get the relevant feedback. Solar missions are the special nexus where scientists both are actively using the |
4f34122
to
8cb5b2f
Compare
If that's the case, isn't it worth PR'ing this to astropy to get eyes on it from the start from the developers of where we want it to end up? Short term it might be easier for us (sunpy develoeprs) to quickly put something in sunpy, but since this is a major new feature I think it's worth putting the effort in to have design decisions with people outside the solar physics community before jumping in, or putting this in another package that is not mature and constrained to a release cycle like sunpy is that would actually allow for faster iteration and development. |
Discussed briefly on the weekly call, and consesnus was to go ahead and put this in sunpy core |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than a few coverage gaps (primarily get_body with an observer by the looks of it) I think this looks good to go.
sunpy/coordinates/spice.py
Outdated
new_pos = shifted_new_pos + CartesianRepresentation(icrs_offset.T) | ||
return to_icrs_frame.realize_frame(new_pos) | ||
|
||
frame_transform_graph._add_merged_transform(spice_frame, ICRS, spice_frame) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Random aside, but we are making heavy use of this method in sunpy, should we open a PR to astropy to make it not private?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, not exactly "heavy use", as this would be the first time that we actually use the method in sunpy
. (We needed our minimum Astropy dependency to be 5.0, and I didn't want to start refactoring until transformations.py
to turned private.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh maybe I am getting confused between your geo frames PR and the merged code lol. Either way, I think that if we are going to rely on it we should ask astropy to mark it as public, mainly so someone doesn't change it somehow thinking that's a safe thing to do?
f46bae5
to
24b657c
Compare
72424ab
to
1ca2c6c
Compare
At the recent DASH meeting, there was a lot interest expressed in having SunPy support SPICE kernels at some level. The now-archived
astrospice
package covered a little of what people use SPICE for, but is still missing many things.This PR wraps the SPICE computations using the API of our coordinates framework (i.e., using
SkyCoord
). Here's an example using Solar Orbiter's predicted set of SPICE kernels:TODO for this PR:
to_helioprojective()
get_fov()
to_helioprojective()
install_frame()
)Deferred to future PRs: