Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Interoperability of rtell IGRINS files with revised uncertainty correction #126

Open
gully opened this issue Feb 13, 2023 · 3 comments
Open
Assignees

Comments

@gully
Copy link
Member

gully commented Feb 13, 2023

@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
Copy link
Member Author

gully commented Feb 13, 2023

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
Copy link
Collaborator

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
Copy link
Collaborator

Okay the uncertainty propagation issues should be fixed now in PR #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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants