Skip to content

Conversation

kfindeisen
Copy link
Member

This PR corrects the definition of our SingleFrame pipelines to be more aligned with ap_pipe's.

The pipelines included analyzeCalibrationMetrics (unknown label?) but
did not include associateSolarSystemDirectSource. Making the pipeline
definition opt-out instead of opt-in should keep it closer to its
intended meaning of "ap_pipe/SingleFrame, without Preprocessing".
SingleFrame.yaml has tasks not found in ApPipe.yaml, so it must be
updated separately.
@kfindeisen kfindeisen requested a review from isullivan April 25, 2025 00:24
Copy link
Member

@isullivan isullivan left a comment

Choose a reason for hiding this comment

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

One question about excluding loadDiaCatalogs

# Run in prep_butler or Preprocessing.yaml
- getRegionTimeFromVisit
- mpSkyEphemerisQuery
- loadDiaCatalogs
Copy link
Member

Choose a reason for hiding this comment

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

It is true we don't want to run loadDiaCatalogs in SingleFrame, but I don't actually see that in the imports so I think the exclusion is unnecessary.

Copy link
Member Author

@kfindeisen kfindeisen Apr 25, 2025

Choose a reason for hiding this comment

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

Yes, it's not there. This was intended as "future-proofing" against changes to SingleFrame (which I think is part of what caused this mess in the first place). We want to include any changes to upstream SingleFrame, except changes that incorporate more of preprocessing.

And of course I tested that excluding a nonexistent task is legal. 🙂

@kfindeisen kfindeisen merged commit c8cd184 into main Apr 25, 2025
11 checks passed
@kfindeisen kfindeisen deleted the tickets/DM-50500 branch April 25, 2025 01:06
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