Cubeviz parser: Refactor parser, add ndarray support, metadata viewer #1040
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1040 +/- ##
==========================================
+ Coverage 76.17% 77.16% +0.99%
==========================================
Files 87 87
Lines 6685 6813 +128
==========================================
+ Hits 5092 5257 +165
+ Misses 1593 1556 -37 ☔ View full report in Codecov by Sentry. |
pllim
force-pushed
the
cubeviz-parse-all-the-things
branch
from
e6876f8 to
265f87d
Compare
| flux = val.array << file_obj.flux.unit | ||
| elif attr == "mask": | ||
| data_type = 'mask' | ||
| flux = val << u.dimensionless_unscaled |
There was a problem hiding this comment.
Mask should not have units. It is basically data quality flags (unitless). Not sure why it was inheriting science data units.
| flux = val | ||
|
|
||
| flux = np.moveaxis(flux, 1, 0) |
There was a problem hiding this comment.
Really not sure what this moveaxis is for. All it did was to make the output X/Y flipped w.r.t. input. I removed it but if there is a reason for it to be there, please let me know.
There was a problem hiding this comment.
I think the reason for this had something to do with preserving the orientation that specutils understands and expects, or handling a difference between specutils and spectral-cube? There was also a related issue of whether or not the input cubes needed to be transposed inside the relevant specutils cube loaders. I can't quite remember. There was some discussion about it here, astropy/specutils#852, and maybe in some other jdaviz PRs/issues. @rosteen might remember more specific details.
There was a problem hiding this comment.
But this line doesn't change where the WCS axis go, just flipping spatial, which is very confusing for me. Maybe I am missing something here.
There was a problem hiding this comment.
I don't remember if this was originally put in for WCS reasons, or something else. There might be unintended consequences by removing it. Maybe someone else can comment on it. Looks like it was put in by @javerbukh during the spectralcube to specutils transition.
There was a problem hiding this comment.
I think @javerbukh said that we no longer needs np.moveaxis.
pllim
requested review from
duytnguyendtn,
ibusko,
javerbukh,
kecnry,
ojustino and
rosteen
as code owners
This comment was marked as resolved.
This comment was marked as resolved.
pllim
force-pushed
the
cubeviz-parse-all-the-things
branch
from
bdbf102 to
5f73486
Compare
pllim
force-pushed
the
cubeviz-parse-all-the-things
branch
from
5f73486 to
e1c0487
Compare
pllim
force-pushed
the
cubeviz-parse-all-the-things
branch
2 times, most recently
from
3b47227 to
ef515b0
Compare
See JP-2280. Except for Cubeviz because that is being refactored at spacetelescope#1040
See JP-2280. Except for Cubeviz because that is being refactored at spacetelescope#1040
and fix tests.
Add metadata viewer to Cubeviz. Better handling of no WCS cases.
for spectral axis in pixels use case
pllim
force-pushed
the
cubeviz-parse-all-the-things
branch
from
8b06bff to
e6a1f31
Compare
|
I think this is too stale now. Also I am not sure if some of the stuff here still applies when "Cubeviz redesign" happens. |
Description
This pull request is to:
Fix moment map result that is rotated.Moved to BUG: Fix transposed moment result #1104TODO
u.pixto "slice". Upstream still uses pixel but the plot won't say that.If that does not happen in time, then PL will emit warning on load for those data and we can defer fixing plugins as future work.(or maybe defer as future PR)Are people happy with hacky way to assign dummy WCS? Is there a better way?specutilsrelease.glue-astronomyrelease. Or copy the code we need over where if it is not going to happen soon?Checklist for package maintainer(s)
This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.
triviallabel.CHANGES.rst?