pypeit / PypeIt

The Python Spectroscopic Data Reduction Pipeline
BSD 3-Clause "New" or "Revised" License
163 stars 104 forks source link

UVES Popler support for coadding data #1830

Closed rcooke-ast closed 2 months ago

rcooke-ast commented 4 months ago

I have added support to combine PypeIt data products with UVES_popler. The UVES_popler changes have now been merged, although there may still be a few minor issues that need to be sorted out. Michael Murphy has kindly joined the User's Slack and is happy to assist users with some support if they choose to use this package. I've also updated some of the docs.

Overall, if this is something that receives a lot of interest, we may want to consider redeveloping a GUI for such data combination in the future, possibly with python so users don't need to install CFITSIO and PGPLOT...

debora-pe commented 4 months ago

It seems a good idea to have the "blaze function" in the SpecObj, but we may want to consolidate this with what pypeit_coadd_1dspec is doing, which also uses the blaze. @jhennawi what do you think?

Also, @rcooke-ast can you add a quick example of how to use UVES_popler with PypeIt spec1ds?

rcooke-ast commented 4 months ago

Thanks for the quick feedback @kbwestfall and @debora-pe! Responding to your queries:

Increment the datamodel version number for SpecObj.

Done!

I dislike our use of the term "blaze function" here. The flat-field observations do not strictly provide the blaze function only (unless I'm missing something). Maybe that's the dominant term, but there are other components (e.g., the detector QE, etc). Why not just use "FLAT" instead?

You're right, it's not strictly the blaze, but this is how UVES_popler reads in the PypeIt files. I have changed this to FLAT, and I'll submit a small PR to UVES_popler.

Also, @rcooke-ast can you add a quick example of how to use UVES_popler with PypeIt spec1ds?

I've linked to the documentation, and I don't wish to reproduce too much here, but I've added some of the first simple steps to get things going. For the record, here is the documentation for your interest:

https://astronomy.swin.edu.au/~mmurphy/UVES_popler/

Also, Michael Murphy has a video tutorial of how to interact with UVES_popler, and is going to share that with us.

codecov-commenter commented 4 months ago

:warning: Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 66.66667% with 30 lines in your changes missing coverage. Please review.

Project coverage is 38.07%. Comparing base (081f2f4) to head (492c13b).

Files with missing lines Patch % Lines
pypeit/specobjs.py 56.52% 10 Missing :warning:
pypeit/core/telluric.py 0.00% 5 Missing :warning:
pypeit/flatfield.py 61.53% 5 Missing :warning:
pypeit/specobj.py 66.66% 3 Missing :warning:
pypeit/coadd1d.py 0.00% 2 Missing :warning:
pypeit/scripts/sensfunc.py 60.00% 2 Missing :warning:
pypeit/core/coadd.py 0.00% 1 Missing :warning:
pypeit/core/wavecal/wvutils.py 50.00% 1 Missing :warning:
pypeit/pypeit.py 66.66% 1 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #1830 +/- ## =========================================== + Coverage 38.02% 38.07% +0.04% =========================================== Files 211 211 Lines 48975 49010 +35 =========================================== + Hits 18625 18662 +37 + Misses 30350 30348 -2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

rcooke-ast commented 3 months ago

Tests pass...

image
rcooke-ast commented 2 months ago

OK - I've unified the approach to the blaze computation, and tests now pass. This also required a few bug fixes in the flatfield code which produces infinities in the flat model (even on the develop branch).

image
rcooke-ast commented 2 months ago

Tests pass...

image
rcooke-ast commented 2 months ago

Tests still pass

image
rcooke-ast commented 2 months ago

I've checked this branch works with UVES_popler, so I'm merging.