glue-viz / glue-astronomy

Plugin to add astronomy-specific functionality to glue
https://glue-astronomy.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
11 stars 12 forks source link

Adding support for uncertainty extraction to NDDataArray #86

Closed bmorris3 closed 1 year ago

bmorris3 commented 1 year ago

In #81, I added translators for objects in astropy.nddata. In this PR, I add support for extracting uncertainties from glue Data objects and translating them into the uncertainty attr in NDDataArray objects. This was missing in the previous PR, but should have been there.

pllim commented 1 year ago

CI failures appear related.

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.34 :tada:

Comparison is base (63d9e0c) 96.99% compared to head (5469ccb) 97.33%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #86 +/- ## ========================================== + Coverage 96.99% 97.33% +0.34% ========================================== Files 17 18 +1 Lines 1296 1351 +55 ========================================== + Hits 1257 1315 +58 + Misses 39 36 -3 ``` | [Impacted Files](https://codecov.io/gh/glue-viz/glue-astronomy/pull/86?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glue-viz) | Coverage Δ | | |---|---|---| | [glue\_astronomy/translators/nddata.py](https://codecov.io/gh/glue-viz/glue-astronomy/pull/86?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glue-viz#diff-Z2x1ZV9hc3Ryb25vbXkvdHJhbnNsYXRvcnMvbmRkYXRhLnB5) | `95.37% <100.00%> (+2.81%)` | :arrow_up: | | [glue\_astronomy/translators/tests/test\_nddata.py](https://codecov.io/gh/glue-viz/glue-astronomy/pull/86?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glue-viz#diff-Z2x1ZV9hc3Ryb25vbXkvdHJhbnNsYXRvcnMvdGVzdHMvdGVzdF9uZGRhdGEucHk=) | `98.56% <100.00%> (+0.14%)` | :arrow_up: | | [glue\_astronomy/tests/test\_spectral\_coordinates.py](https://codecov.io/gh/glue-viz/glue-astronomy/pull/86?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glue-viz#diff-Z2x1ZV9hc3Ryb25vbXkvdGVzdHMvdGVzdF9zcGVjdHJhbF9jb29yZGluYXRlcy5weQ==) | `100.00% <0.00%> (ø)` | | | [glue\_astronomy/spectral\_coordinates.py](https://codecov.io/gh/glue-viz/glue-astronomy/pull/86?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glue-viz#diff-Z2x1ZV9hc3Ryb25vbXkvc3BlY3RyYWxfY29vcmRpbmF0ZXMucHk=) | `100.00% <0.00%> (+6.66%)` | :arrow_up: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glue-viz). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=glue-viz)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

dhomeier commented 1 year ago

Looks good for StdDevUncertainty. I admit I don't know exactly to what extent glue Data objects support other uncertainty types, but Specutils1DHandler processes and stores the types and units to retrieve the appropriate type: https://github.com/glue-viz/glue-astronomy/blob/71ded78d9d2b425aad416ab6d734393f0912ef77/glue_astronomy/translators/spectrum1d.py#L275-L277

It would be good to cover this consistently, and perhaps add test cases for VarianceUncertainty and InverseVariance. Then at some point we should probably see how the Spectrum1D translator can reuse the NDData functionality, but that need not be part of this PR.

bmorris3 commented 1 year ago

@dhomeier I agree this would be great! For now, the relevant collapse functions I wrote for jdaviz via astropy in NDDataArray only support variance-like uncertainties, so this PR is a good first step (https://github.com/astropy/astropy/pull/14175).

dhomeier commented 1 year ago

I see. Would it make sense to have to_data convert other types with represent_as(StdDevUncertainty) then? That's probably a bit double-sided, as other applications may have use for the original forms, even if jdaviz does not a this point. But otherwise I think it should check for the correct type on input, as the present implementation would e.g. silently interpret an InverseVariance as StdDevUncertainty and return it as such.

bmorris3 commented 1 year ago

Good idea @dhomeier! Implemented in https://github.com/glue-viz/glue-astronomy/pull/86/commits/91d8dbc8989871182d358dbd5fd46ccc7e29cdc2.

dhomeier commented 1 year ago

Will need imports for the other uncertainties; also we may have to compare to spec.uncertainty instead of uncertainty since the former should have the correct units set (not sure if represent_as will transform it correctly otherwise).

bmorris3 commented 1 year ago

@dhomeier Done and passing 💯

bmorris3 commented 1 year ago

@dhomeier Would you consider this a bugfix, for soon release? We will use it downstream in https://github.com/spacetelescope/jdaviz/pull/2039.