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

DM-43340: Add exposure pairing for full array mode #227

Merged
merged 6 commits into from
Apr 23, 2024

Conversation

jmeyers314
Copy link
Collaborator

PR adds two new subtasks:
ExposurePairer : automatically pairs intra/extra exposures based on focusz and proximity in time/space/rotation.
TablePairer : pairs intra/extra exposures based on a pre-ingested table of exposure IDs.

Also included are scripts to ingest above table, and modifications to CutoutDonutsScienceSensorTask to use the pairers.

@jmeyers314 jmeyers314 force-pushed the tickets/DM-43340 branch 2 times, most recently from b18e855 to 86be05f Compare April 4, 2024 23:03
Copy link
Member

@jbkalmbach jbkalmbach left a comment

Choose a reason for hiding this comment

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

This is really cool, thanks for adding this in!

Can you please update the doc/uml/taskClass.uml file and the developer guide page?

Some pair tables and butler questions (these might kind of overlap but I'm just writing them down as they occurred to me reading the code):

  • How do you envision pair tables being created/stored in production?
  • Do we want to ingest pair tables for every run into their own collection or would we have a general pair table that is regularly updated and overwritten? Are we then going to have lots of collections with just a pair table as their contents?
  • How we will know which pair tables to use when?

python/lsst/ts/wep/task/cutOutDonutsScienceSensorTask.py Outdated Show resolved Hide resolved
python/lsst/ts/wep/task/cutOutDonutsScienceSensorTask.py Outdated Show resolved Hide resolved
python/lsst/ts/wep/task/pairTask.py Outdated Show resolved Hide resolved

# For each extra focal exposure, find the best intra focal exposure.
# Note that it's possible that the same intra exposure is paired with
# multiple extra exposures.
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to allow the same intra exposure to be paired with multiple extra exposures? Would it be better to create a copy of the intra table and remove rows as they are paired?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's an interesting idea. I'm on the fence as to whether that should be the only mode of operation, or if this should be a config setting. Do you have a preference?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I'm all for keeping our options open and using a config to set this.

@jmeyers314 jmeyers314 force-pushed the tickets/DM-43340 branch 6 times, most recently from dbcb974 to 47c7cef Compare April 15, 2024 18:56
@jmeyers314 jmeyers314 force-pushed the tickets/DM-43340 branch 3 times, most recently from 4378ed8 to 41fe47d Compare April 15, 2024 21:41
@jmeyers314
Copy link
Collaborator Author

Responses to general questions:

How do you envision pair tables being created/stored in production?

We'll have to experiment. It might turn out that we can mostly rely on the automatic pairer. One option we could think about is creating a script that runs the automatic pairer and writes the result to a CSV file that we can then go and tweak by hand. I'm happy to do that here if you like, or leave it for a future ticket.

Do we want to ingest pair tables for every run into their own collection or would we have a general pair table that is regularly updated and overwritten?

Yeah, that's a good question. One pair table per collection is probably the easiest right now; I don't think butler products generally work very well with edit-in-place in mind, though you can mimic that with chain-collections where newer runs shadow older runs.

Are we then going to have lots of collections with just a pair table as their contents?

Possibly many run-collections. Hopefully fewer chain-collections though.

How we will know which pair tables to use when?

I think maintaining a default pair table chain-collection may make sense here. Quick estimate might be ~500 pairs per night and ~100 nights of commissioning. I don't think having a 50,000 row pair table is that bad.

Off-main pairing experiments will have to maintain their own collections. (or develop other Pairer subtasks).

@jmeyers314
Copy link
Collaborator Author

Hey Bryce,
I think I've addressed all your comments. Do you want to take a quick peak again? I also added a "generatePairTable.py" script to help assemble a table that can then be edited manually when using the TablePairer. Thanks!

Copy link
Member

@jbkalmbach jbkalmbach left a comment

Choose a reason for hiding this comment

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

Thanks for the generate script, all my comments have been addressed. Just one comment on the UML that I think is needed based upon what we've added before.

@@ -42,4 +42,6 @@ GenerateDonutFromRefitWcsTaskConfig *-- GenerateDonutFromRefitWcsTaskConnections
GenerateDonutCatalogWcsTaskConfig <|-- GenerateDonutFromRefitWcsTaskConfig
GenerateDonutFromRefitWcsTask *-- GenerateDonutFromRefitWcsTaskConfig
GenerateDonutCatalogWcsTask <|-- GenerateDonutFromRefitWcsTask
ExposurePairer *-- ExposurePairerConfig
TablePairer *-- TablePairerConfig
Copy link
Member

Choose a reason for hiding this comment

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

I think CutOutDonutsScienceSensorTaskConfig *-- ExposurePairer needs to be added as well.

@jmeyers314 jmeyers314 merged commit fd5d5db into develop Apr 23, 2024
3 of 4 checks passed
@jmeyers314 jmeyers314 deleted the tickets/DM-43340 branch April 23, 2024 22:26
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.

3 participants