mosviz niriss parser: transpose R 2d spectra #1619
Conversation
| @@ -708,7 +708,15 @@ def mos_niriss_parser(app, data_dir): | |||
| # The wavelength is stored in a WAVELENGTH HDU. This is | |||
| # a 2D array, but in order to be able to use Spectrum1D | |||
| # we use the average wavelength for all image rows | |||
| wav = temp[wav_hdus[sci]].data.mean(axis=0) * u.micron | |||
|
|
|||
| if orientation == 'R': | |||
There was a problem hiding this comment.
alternatively, should we not rely on this "orientation" and instead assume that the wavelength axis should be longer and change this check to if data.shape[0] > data.shape[1]?
There was a problem hiding this comment.
I think it is fair to assume that the dispersion axis is longer than the cross-dispersion axis and it will be simpler to adapt to other instruments that do not use the R/C nomenclature.
There was a problem hiding this comment.
ok, I switched to that logic. But note that this is currently only implemented for the niriss parser (although we could copy similar logic elsewhere or consider a refactor to have this somewhere more general if needed down the road). Thanks!
There was a problem hiding this comment.
Yes, but it will soon be time to extend to NIRCam ;)
kecnry
force-pushed
the
niriss-r-transpose
branch
from
c510e8e to
75c2949
Compare
Codecov ReportBase: 86.35% // Head: 86.35% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1619 +/- ##
=======================================
Coverage 86.35% 86.35%
=======================================
Files 94 94
Lines 9407 9410 +3
=======================================
+ Hits 8123 8126 +3
Misses 1284 1284
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
kecnry
requested review from
duytnguyendtn,
rosteen,
javerbukh,
ojustino and
pllim
as code owners
kecnry
force-pushed
the
niriss-r-transpose
branch
from
75c2949 to
bb143e6
Compare
There was a problem hiding this comment.
I can see the flip in MosvizNIRISSExample.ipynb's 2D, R-oriented spectra on main and also see that it's fixed here. I didn't encounter any new issues while loading data. If @camipacifici says the length assumption for wavelength axes is fair, that works for me, too.
I have a small comment about the changelog but don't think it should prevent an approval.
CHANGES.rst
Outdated
| @@ -49,6 +49,8 @@ Imviz | |||
| Mosviz | |||
| ^^^^^^ | |||
|
|
|||
| - R-grism data are now loaded with the correct orientation. [#1619] | |||
There was a problem hiding this comment.
I recommend including a "2D" descriptor to be more specific about the resolved issue.
There was a problem hiding this comment.
good idea, added to the changelog and will merge once CI passes. Thanks!
* currently only in niriss parser since occurs for R-grism data * check is based on the assumption that the dispersion direction should be longer than the cross-dispersion
kecnry
force-pushed
the
niriss-r-transpose
branch
from
bb143e6 to
afbbc6f
Compare
Description
This pull request transposes the input data before creating the Spectrum1D object for 2d spectra for all R grism inputs.
Fixes #1353
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?