search

OttoStruve / muler

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

Fix reading IGRINS uncertainty #89

Closed kfkaplan closed 1 year ago

kfkaplan commented 2 years ago

There was an issue in how the IGRINS uncertainty was read in. Previously IGRINS uncertainty was read from the .sn.fits files that the IGRINS PLP outputted. The .sn.fits files give the signal-to-noise per resolution element, NOT per pixel.

You can see where this happens in the IGRINS PLP here (https://github.com/igrins/plp/blob/8fbbf237e2c918f17086ef287cbad5c23d485d35/igrins/recipes/recipe_extract.py#L944)

This resulted in incorrect uncertainty being calculated per pixel. I have modified the code in igrins.py to instead read in the .variance.fits files outputted by the IGRINS PLP instead of the .sn.fits files, which give the variance per pixel. The square root of the variance (per pixel) is then used for the uncertainty.

I have confirmed this fix works and gives the correct uncertainty as shown below. The top figure is a plot of the H I Br-gamma line from a PNe as read in and processed through my old "plotspec" code. The bottom figure is a plot of the same line as from the same IGRINS data read in and processed by Muler. You can see with this fix, the signal-to-noise on the top right of both plots is identical. Both codes are independent of each other and developed years apart, and with this fix, give the same signal-to-noise. HI_1 HI_2

gully commented 2 years ago

Hi @kfkaplan Thank you again for your contributions! Our community is indebted to you 🙏

I will be able to review this and your other Pull Requests on Wednesday, April 20 or thereafter. If you are interested, I propose an interactive modality where we share screen over Zoom to discuss and adapt in near-real time, a mini hackathon/sprint activity. That mode is not a requirement, just an option if it interests you. If interested, we can coordinate through email.

Thank you for your patience while we merge all of your valuable contributions.

gully commented 2 years ago

Note that this is a backwards breaking change: requiring .variance files instead of .sn files, so a few other things will have to be updated: the example data should now house the .variance file instead of (or in addition to) the .sn file. Any users currently using IGRINS data (mostly @ericasaw ) should acknowledge the change and update their workflows if necessary.

kfkaplan commented 2 years ago

Both the .variance.fits and .sn.fits files are generated by the IGRINS pipeline, so if you have one, you should have the other.

I would definitely be interested in having a zoom meeting this week to work on muler.

gully commented 2 years ago

Ok, so the tests are now failing for a good and informative reason. In the switch to variance.fits files, the logic now implicitly assumes that a .spec.fits file was handed in as the input file type. If instead the .spec_a0v.fits suffix files are passed in, the variance no longer applies.

There are a few ways to deal with this. One strategy could be to stick with the .sn.fits files, and simply reverse the scaling that was applied to it to obtain SNR per pixel rather than SNR per res-el. I think this solution should work regardless of whether the user passed in a spec_a0V.fits or .spec.fits file. (I could be wrong on that latter point: whether-or-not the observed A0V spectrum uncertainty got propagated into the SNR).

gully commented 2 years ago

By the way, this friction point reopens an old question: whether and how to support both the .spec.fits files and spec_a0V.fits files.

Previously I had been inclined to focus on the spec_a0V.fits files since they are more user friendly and easy for quick look. But now I see just how crude the out-of-the-box A0V correction can be. I think supporting both approaches in muler would be a huge boon to users, but we have to be disciplined about what-goes-where in order to prevent miscues.

A worked example with the .spec.fits files would go a long way towards bridging the gap between the two methods.

kfkaplan commented 2 years ago

I'm finally coming back to this and it's been a while so I've had to remind myself what was going on.

There are two issues. One is .sn.fits storing the uncertainity per resolution element instead of per pixel. That's something that needs fixing and will be fixed once this pull request is merged with main. The code in its current form has the main branch and all of its derivatives currently using .sn.fits while my branch fix_igrins_uncertainity is using .variance.fits. In the end, it doesn't really matter which is used since the resulting uncertainty is the same once you divide out the "per resolution element" from the uncertainty stored in .sn.fits. This is all independent of if your input spectrum is a .spec.fits or spec_a0V.fits file. I think the solution is to just automate the whole process to work for all use cases. First search for .variance.fits and if it is not found, use .sn.fits. It should be as simple as that. The user doesn't even have to think about it. I will work on updated the code to do this in the fix_igrins_uncertainity which should then pass the tests for this pull request when it is finished.

kfkaplan commented 2 years ago

Hmm this is very strange. The tests with astropy 5.0.1 fail but seem to be running on the wrong commit and are failing in ways that should have been fixed. I do not really know what is going on here. I will have to investigate.

kfkaplan commented 2 years ago

Upon further investigation:

On my local machine, pytest runs fine.

Pytest as a github action here seems to be running on the main branch and not fix_igrins_uncertainity. Obviously I would like the tests to run on the code pushed to the branch I am working on, and not on main. Specifically src/muler/echelle.py from the main branch is being grabbed instead of fix_igrins_uncertainity. Will need to figure out how to get the tests to run on the branch that is pushed and not main.

kfkaplan commented 2 years ago

@gully Any idea on why these tests appear to be running only on the main branch on not on the branch and commit I am pushing? I've tried many different ways to fix this but I can't figure it out. Are the tests only designed to run for the main branch? Is setup.py grabbing the release tagged v0.3.4 and that is what is going on?

gully commented 1 year ago

I'm going to dig into the unit test questions as part of a versioning health check...

gully commented 1 year ago

Hi @kfkaplan, yes the GitHub Action tests were erroneously running the pip version of muler for the unit tests, because muler resided in the docs/requirements.txt file. I fixed the issue so now GitHub Actions should test the development version. Thanks!

kfkaplan commented 1 year ago

@gully Thanks for the fix. I merged the current version of main with fix_igrins_uncertainity and that fixed the GitHub Action testing issues https://github.com/OttoStruve/muler/issues/121. I also modified igrins.py to be able to handle spec_flattened.fits files (which the IGRINS PLP generates for standard stars) and tried to merge in reading in rtell files as well. Now it looks like this branch passes all the tests!

kfkaplan commented 1 year ago

One thing I am just a little unclear about, do "rtell" files calculate their signal-to-noise per spectral resolution or pixel? This whole pull request is meant to fix the uncertainty to be per pixel so I want to make sure I have it right for those files. Currently my code treats it as per pixel.

kfkaplan commented 1 year ago

We just reviewed this in person and it looks good to go. Please raise any issues if you encounter problems.