search

OttoStruve / muler

A Python package for working with pipeline-produced spectra from IGRINS, HPF, and Keck NIRSPEC
https://muler.readthedocs.io
MIT License
14 stars 9 forks source link

Interoperability of rtell IGRINS files with revised uncertainty correction #126

Open gully opened 1 year ago

gully commented 1 year ago

@ericasaw reports that PR #89 affected the behavior of rtell files, with a shift in the uncertainty scaling. Tracking down the cause of the discrepancy was tricky because there are so many conditional control flows (nested if statements).

I propose a solution to refactor and modularize these into three main IGRINS use cases: We should have two or three separate functions: init_rtell_spectrum(fn) and init_spec_spectrum(fn) and init_spec_A0V_spectrum(fn) or equivalent names.

Then we can simplify the sequencing substantially:

if "rtell" in fn:
    output_spectrum = `init_rtell_spectrum(fn)`
# and so on [...]

There will be significant code duplication among these three functions, but that's OK for now. We can modularize some of those subroutines later. But currently the intermingling of the scenarios is making it tricky to see what is happening at-a-glance.

This fix is aimed at code sustainability and readability.

gully commented 1 year ago

Kyle I "assigned" you just to keep you in the notification loop, and because you were the author of the PR, but really anyone is invited to work on this if you have the time and energy.

kfkaplan commented 1 year ago

I am currently fixing some issues with the signal-to-noise files (and S/N stored in the rtell files) propagating the correct uncertainty. I think I have a fix ready for later today. It is all intertwined with this mess of nested if statements I will submit a pull request and then in the same branch we can look into modifying the code for this, but please wait until the signal-to-noise issue is fixed later today so we don't have the code diverge.

kfkaplan commented 1 year ago

Okay the uncertainty propagation issues should be fixed now in PR https://github.com/OttoStruve/muler/pull/89. My tests show I get the same uncertainty no matter what combination of .spec.fits, .spec_a0v.fits, .sn.fits, .variance.fits, and rtell files are used (rtell files combine the uncertainty of the standard star with the science target so if the science target is bright, the SNR will be lower). Please feel free to test the branch in the PR.

@gully to be clear, are you suggesting IGRINSSpectrumList and IGRINSSpectrum classes be split into three different versions to handle the .spec.fits, .spec_a0v.fits, and rtell files?