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 NDData support as data factories to glue #2164

Closed
wants to merge 7 commits into from
Closed

Add NDData support as data factories to glue #2164

wants to merge 7 commits into from

Conversation

kakirastern
Copy link

Description

Add astropy.nddata support as data factories in order to offer an alternative means to load more specific data types, such as FITS files, but using the NDData, making the manipulation of units, mask, uncertainty, and meta of any FITS data more manageable in the context of glue.

@codecov
Copy link

codecov bot commented Jul 25, 2020

Codecov Report

Merging #2164 into master will decrease coverage by 0.00%.
The diff coverage is 84.84%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2164      +/-   ##
==========================================
- Coverage   87.87%   87.87%   -0.01%     
==========================================
  Files         246      247       +1     
  Lines       22724    22788      +64     
==========================================
+ Hits        19969    20024      +55     
- Misses       2755     2764       +9     
Impacted Files Coverage Δ
glue/core/data_factories/nddata.py 77.14% <77.14%> (ø)
glue/core/data.py 90.71% <93.33%> (+0.07%) ⬆️
glue/core/data_factories/__init__.py 100.00% <100.00%> (ø)
glue/__init__.py 70.37% <0.00%> (+3.70%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cf93252...0b089d7. Read the comment docs.

Copy link
Member

@astrofrog astrofrog left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! This introduces a very big change which is to add mask and uncertainty to the main data object, which I don't think we want to do. In particular, each attribute could in principle have an uncertainty. I think a better approach would be for now to store the mask and uncertainty in attributes with special names which then doesn't require any changes to the main data class.

This also overlaps discussion wise with glue-viz/glue-astronomy#17 - which is about translating NDData <-> Data and once that is complete we can leverage that to simplify the FITS as NDData reader. However, I think that to make things easier to manage, this data factory here should also be added to glue-astronomy, not glue-core (to keep all the NDData stuff in a single place)

@astrofrog
Copy link
Member

More generally, I wonder if we should just be doing NDData.read here and making that work properly for FITS in astropy. Alternatively, glue-solar could implement factories that delegate to <some NDData subclass>.read

@kakirastern
Copy link
Author

Sure, let me make the changes soon

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.

None yet

2 participants