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

Fix attribute handling for Jdaviz/Cubeviz parser #56

Closed pllim closed 2 years ago

pllim commented 2 years ago

Description

This PR fixes regression in Cubeviz parser behavior introduced in spacetelescope/jdaviz#547 , particularly getting rid of the assumption that "uncertainty" is always available when "flux" is available, which is not always true.

~Also refactor the internal logic a little.~

Fixes spacetelescope/jdaviz#967

codecov[bot] commented 2 years ago

Codecov Report

Merging #56 (6159fef) into main (5b8a32c) will not change coverage. The diff coverage is 100.00%.

:exclamation: Current head 6159fef differs from pull request most recent head 4c1466a. Consider uploading reports for the commit 4c1466a to get more accurate results Impacted file tree graph

@@           Coverage Diff           @@
##             main      #56   +/-   ##
=======================================
  Coverage   96.03%   96.03%           
=======================================
  Files          15       15           
  Lines        1136     1136           
=======================================
  Hits         1091     1091           
  Misses         45       45           
Impacted Files Coverage Δ
glue_astronomy/translators/spectrum1d.py 92.56% <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 5b8a32c...4c1466a. Read the comment docs.

pllim commented 2 years ago

Failures might be related. I might have broken some other stuff in making Cubeviz work. 🤷

I'll investigate tomorrow.

pllim commented 2 years ago

The code is too sensitive. There are Glue subtleties that I am not familiar with, so any small refactoring breaks stuff. I have undone the refactoring and just left in a minimal fix that would hopefully fix Cami's use cases.

pllim commented 2 years ago

py36 failure is unrelated. I don't think you can get Python 3.6 on Azure anymore but fixing that is out of scope.

pllim commented 2 years ago

@astrofrog or @eteq , any chance you can merge and do a release with this fix?

pllim commented 2 years ago

Thanks for the review, @rosteen !