desihub / prospect

DESI spectral visualization tools
BSD 3-Clause "New" or "Revised" License
11 stars 6 forks source link

Merge plotspecutils #66

Closed armengau closed 3 years ago

armengau commented 3 years ago

Not fully ready yet for merge, but ready for review.

viewer.py is the result of a manual merge between plotframes.py and plotspecutils.py. I kept the most up-to-date code from both, with minimal changes.

Input spectra in viewer.plotspectra() can be Spectra or list[Frame] or Spectrum1D or SpectrumList. Key changes are at the beginning of plotspectra(), and in make_cds_spectra().

As of now I made tests using DESI spectra only:

@weaverba137 Could you in particular:

coveralls commented 3 years ago

Pull Request Test Coverage Report for Build 513356796


Changes Missing Coverage Covered Lines Changed/Added Lines %
py/prospect/plotspecutils.py 0 1 0.0%
py/prospect/plotframes.py 0 2 0.0%
py/prospect/viewer.py 0 784 0.0%
<!-- Total: 0 787 0.0% -->
Files with Coverage Reduction New Missed Lines %
py/prospect/plotframes.py 1 0%
py/prospect/plotspecutils.py 1 0%
<!-- Total: 2 -->
Totals Coverage Status
Change from base Build 496910645: -0.4%
Covered Lines: 59
Relevant Lines: 3942

💛 - Coveralls
weaverba137 commented 3 years ago

I won't have time to review until next week, but thank you for getting this out.

weaverba137 commented 3 years ago

With a minor typo fix, the viewer.py code is working in a Data Lab notebook. However there is still an issue with the units of the model spectrum which is preventing plotting SDSS models. That won't be hard to fix, I've seen it before.

I still want to do a line-by-line code comparison to make sure nothing subtle has been missed, but overall, I think things are working.

weaverba137 commented 3 years ago

The problem with units on the model is fixed on the Data Lab side, it doesn't need a fix in this code.

armengau commented 3 years ago

About flux units: I suppose that flux units will never change in DESI. I had already noticed that, while the unit is present in the fits file: eg fits.getheader('/global/cfs/cdirs/desi/spectro/redux/daily/tiles/68000/20200314/coadd-0-68000-20200314.fits','brz_flux')['bunit'] we apparently cannot recover it from the Spectra objects. Well, that would be a request to desispec: in desispec.io.read_spectra() put flux units somewhere in meta or extra?

weaverba137 commented 3 years ago

In this particular case, it's not a case of whether the flux array has (Astropy) units, but what scaling is applied to those units. Prospect expects 1e-17 erg cm^-2 s^-1 A^-1, but the Data Lab spectrum service returns erg cm^-2 s^-1 A^-1, so it's just a matter of multiplying or dividing by 1e-17 as needed.