search

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 13 forks source link

Store correct units for uncertainty in Glue data object #25

Closed rosteen closed 4 years ago

rosteen commented 4 years ago

Small fix, closes #24

rosteen commented 4 years ago

@astrofrog @eteq, this should be quick to confirm and merge. I think it will fix some failing tests I'm chasing down over in jdaviz.

codecov[bot] commented 4 years ago

Codecov Report

Merging #25 into master will decrease coverage by 0.00%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #25      +/-   ##
==========================================
- Coverage   97.17%   97.17%   -0.01%     
==========================================
  Files          13       13              
  Lines         957      956       -1     
==========================================
- Hits          930      929       -1     
  Misses         27       27
Impacted Files Coverage Δ
glue_astronomy/translators/spectral_cube.py 100.00% <ø> (ø)
glue_astronomy/translators/ccddata.py 100.00% <100.00%> (ø)
glue_astronomy/translators/spectrum1d.py 97.22% <100.00%> (ø)
...lue_astronomy/translators/tests/test_spectrum1d.py 100.00% <100.00%> (ø)

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 b518b0d...dc3494a. Read the comment docs.

rosteen commented 4 years ago

Yes, good call - I'll do that after I'm out of meetings this afternoon.

rosteen commented 4 years ago

@eteq I updated the tests to use an uncertainty type with different units than the flux, which would have caught this error. There are some unrelated tox failures - I fixed one, do you want me to fix the rest here or do that in a followup later?

eteq commented 4 years ago

Awesome, I think this is good as it stands. I think the unrelated issue should be fixed separately (in another PR) since I agree it looks independent.