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

Update to branch before adding detection module #10

Merged
merged 19 commits into from
Mar 28, 2023

Conversation

ricolandman
Copy link
Contributor

Finally found the time to add all changes properly. What I added:

  • obs_nodding_irregular: Function that works for any nodding sequence (I needed this for my own transit observations). I wanted to implement this within obs_nodding but can't think of a simple way to do it. Right now it is a lot of duplicate code, so could definitely be improved in the future.
  • util_extract_science: Function that extracts the results from obs_nodding for specific slit_fraction (e.g. for widely separated planets).
  • 2nd order wavelength correction. This "grid search" is quite memory intensive, so I run "correct_wavelengths" multiple times with decreasing accuracy to make it feasible.
  • Changed initial guess in fit_gaussian to make the function also works when the star is not in the center.
  • custom_extract_2d: Including correction for slit tilt. This makes sure the wavelength solution is consistent vertically, so that we can sum over the spatial dimension in correct_wavelengths_2d.
  • obs_nodding: Added the option "check_existing" if you want to skip already reduced files.

I tested it on the beta Pic data and some transit observations and it all runs smooth, but can't guarantee that there are no bugs. Feel free to change stuff or let me know if you want things changed. I have started on the detection module, which should come soon.

@ricolandman
Copy link
Contributor Author

Oops, I see I messed up the branches. This actually already includes the functions for planet detection:

  • remove_starlight: Function for removing the stellar contribution from each row
  • remove_pca: Function for removing PCA components for each row
  • cross_correlate: Function for cross-correlation.

While these functions are definitely not optimized (I can get a much higher S/N detection with my custom method) you should be able to detect e.g. beta Pic b quite easily using these functions.

@tomasstolker tomasstolker self-requested a review March 24, 2023 13:19
Copy link
Owner

@tomasstolker tomasstolker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing work!! Many thanks @ricolandman!

I tested the new methods and it runs all fine. I did not check all the code in detail but from what I can tell it all looks good! Just added a few minor comments. The only larger request that I would have is to add some brief docstrings to the util functions if you have the time for that.

Does the new obs_nodding_irregular method also work on a regular/ABBA nodding pattern? If so then feel free to remove the old obs_nodding and use obs_nodding_irregular as the new obs_nodding 👍.

Let me know in case you have any questions!

pycrires/util.py Show resolved Hide resolved
pycrires/util.py Show resolved Hide resolved
pycrires/pipeline.py Show resolved Hide resolved
pycrires/pipeline.py Outdated Show resolved Hide resolved
pycrires/pipeline.py Outdated Show resolved Hide resolved
pycrires/pipeline.py Outdated Show resolved Hide resolved
pycrires/pipeline.py Outdated Show resolved Hide resolved
pycrires/pipeline.py Outdated Show resolved Hide resolved
pycrires/pipeline.py Outdated Show resolved Hide resolved
pycrires/pipeline.py Outdated Show resolved Hide resolved
@ricolandman
Copy link
Contributor Author

Thanks! The obs_nodding_irregular should also work on regular ABBA nodding cycles, but is a factor two slower as it loops over all files instead of just the A files. I guess this could be avoided with a simple check though. Will think about it some more.
Thanks for the suggestions, I will implement them!

@tomasstolker
Copy link
Owner

Thanks a lot for the additional improvements!

The checks are not passing. I think because the requirements.txt may not be complete?

Copy link
Owner

@tomasstolker tomasstolker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for all your work @ricolandman! The PR seems ready to be merged 🎉

@tomasstolker tomasstolker merged commit 463a0b2 into tomasstolker:main Mar 28, 2023
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

Successfully merging this pull request may close these issues.

None yet

2 participants