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

Enable loading to and saving from KPFITS format #22

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from
Open

Enable loading to and saving from KPFITS format #22

wants to merge 8 commits into from

Conversation

vandalt
Copy link
Collaborator

@vandalt vandalt commented Jul 5, 2023 •
edited
Loading

This enables xara to load data from a KPFITS file and to save results to KPFITS. The format used follows @kammerje et al. 2023 (https://ui.adsabs.harvard.edu/abs/2023PASP..135a4502K/).

There are two things left as TODO in the code which should be addressed before merging:

  • What should we do with the list/datasets dimension in Xara when saving a result to KPFITS? Do multiple KPFITS files, all merge in a single one? I'd be in favor of collapsing this dimension I think, with a warning to say "we're collapsing the dataset dimension, this might not be what you expected".
  • What should be the default format when loading data now, the "LEGACY" FITS format or "KPFITS"? I put KPFITS but was not sure about that, should we set default to None and try both by default?

Fixes #20.

I think this is the last change I had locally, I'll work on merging this fork with the main one once this is merged.

@vandalt
Add code to load KPFITS format
4f72fd2 
Using KPFITS as defined by Kammerer et al. 2022 (NIRISS KPI Paper).
Used KPFITS as new default format, but with a TODO to remind to discuss on PR.
KPFITS did not (in my understanding) specify the "list" level in xara, so loading all in single level.
@vandalt
Add function to save results as KPFITS file
2f6c232 
Used the Kammerer et al. 2022 specification.
Kept the first xara "list" dimension with a TODO comment. Should be discussed in PR.
@vandalt vandalt requested a review from kammerje July 5, 2023 12:51
@kammerje
Copy link
Owner

Thanks a lot @vandalt, it is great to see KPFITS file functionality for XARA. Regarding your points, KPFITS does support several frames per file, so I think the list dimension should simply be collapsed into a single KPFITS file with multiple frames. For the default format I would choose the legacy format, so that already existing code doesn't crash once people update to the new version of XARA. Would you agree?

@vandalt
Copy link
Collaborator Author

vandalt commented Jul 12, 2023

Thanks for the feedback @kammerje. I updated with the changes. I left a TODO comment about potentially using KPFITS as default in the future, and I added a warning about collapsing to the frame dimension (so that if people expect multiple KPFITS or something along the wavelength axis they know that's not what's happening).

@vandalt vandalt mentioned this pull request Jul 12, 2023
Draft
@vandalt
Copy link
Collaborator Author

vandalt commented Jul 12, 2023

I found inconsistencies in the shape. Fixing them just now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kammerje kammerje Awaiting requested review from kammerje

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@vandalt @kammerje