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

adding new rubin ToO strategy into nmma #363

Merged

Conversation

bking-astro
Copy link
Collaborator

Added the updated Rubin ToO strategy into nmma/em/injection.py and nmma/em/analysis.py. Strategies were taken from the 2024 Rubin ToO workshop (https://lssttooworkshop.github.io/images/Rubin_2024_ToO_workshop_final_report.pdf). I will note here that we only use the 120s exposure limiting magnitudes. I think this is not an issue because Night 0 for the gold strategy and Night 1,2,3 for the silver strategy use 120s exposures. The gold Nights 1,2,3 use the 180s exposure but there is only a change of ~0.2 mag in the limiting magnitudes when going from 120s to 180s. The Night 0 silver strategy uses 30s exposure but the Night 0 observation should always be brighter than Nights 1,2,3. Let me know if I should incorporate different limiting mags for different nights.

@sahiljhawar
Copy link
Member

@bking-astro can you also change the relevant flags in tests or create new tests within the same files? see here for an example:

def test_analysis_sklearn_gp(args):

@bking-astro
Copy link
Collaborator Author

I added the ignore_timeshift flag. All the other flags in the test shouldn't be affected because they have rubin_ToO = False and rubin_ToO_type=None.

@sahiljhawar
Copy link
Member

sahiljhawar commented Jun 11, 2024

@bking-astro I have added review comments in the Files changed section

nmma/em/utils.py Outdated Show resolved Hide resolved
nmma/em/analysis.py Show resolved Hide resolved
nmma/em/analysis.py Show resolved Hide resolved
@sahiljhawar
Copy link
Member

sahiljhawar commented Jun 11, 2024

LGTM!

incosistent tab length (linting issues)
incosistent tab length (linting issues), again!
@mcoughlin
Copy link
Member

@tsunhopang can you please review and merge?

Copy link
Collaborator

@tsunhopang tsunhopang left a comment

Choose a reason for hiding this comment

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

LGTM

@tsunhopang tsunhopang merged commit 4ee1f3e into nuclear-multimessenger-astronomy:main Jul 9, 2024
4 checks passed
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.

4 participants