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

tickets/PREOPS-4640: Support simultaneus viewing of multiple simulations in schedview-prenight #106

Merged
merged 24 commits into from
Sep 24, 2024

Conversation

ehneilsen
Copy link
Collaborator

No description provided.

Copy link
Member

@rhiannonlynne rhiannonlynne left a comment

Choose a reason for hiding this comment

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

I have to say -- I haven't taken the time to properly leisurely read the maths and investigate the plotting results. I am glad there is a unit test which should catch basic problems with the matches. It might be good if one of the sequences was such that a field never showed up at all (I can't quite tell if that happens or not), and then checked that the final result was appropriate.

I'm going to go over and look at the notebook that results, and I figure you can update here if needed.



def often_repeated_fields(visits: pd.DataFrame, min_counts: int = 4):
"""Find often repeated fields in a table of visits.
Copy link
Member

Choose a reason for hiding this comment

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

Can you make some comment in the docstring or in the code about what this is for? Why four visits to a given field for a "often visited field"?
In operations sims, four visits to a field would be something that was either a DDF, a near-sun twilight microsurvey visit, or a field which was caught during twilight and again during the night .. we shouldn't generally be covering other fields more than that. In auxtel sims now, four visits to a field is not uncommon.
Between these two use-cases, I'm confused what this is for.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, my intention was to exclude most WFD fields, but include fields where we spend a lot of time.
The idea is to provide a single table for people interested in specific fields without producing something too long to be convenient. That is, I expect that it will be much more common for a user to want to know "how many times are we likely we to observe this specific DDF field tonight" than it is for a user to want to know about any specific WFD field.
We might want something to do something similar for larger areas of WFD coverage, and I have a few ideas, but none I'm really happy with. This is something to be added later.

----------
visits : `pd.DataFrame`
A table that must include both the columns listed in
``sim_identifier_column`` and ``visit_spec_columns`` (below).
Copy link
Member

Choose a reason for hiding this comment

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

I'd typically call "visits" the visits from one simulation, so it may be worthwhile to add here something like
"This data frame holds the visits read from multiple simulations, concatenated together."

@ehneilsen ehneilsen merged commit acac144 into main Sep 24, 2024
7 checks passed
@ehneilsen ehneilsen deleted the tickets/PREOPS-4640 branch September 24, 2024 17:37
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.

2 participants