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-26140: Standardize AP pipeline structure #89

Merged
merged 5 commits into from Oct 8, 2021
Merged

Conversation

kfindeisen
Copy link
Member

This PR reorganizes the ap_pipe pipeline structure slightly to make it easier to call from ap_verify. The only functional change should be the creation of new subsets.

Copy link
Collaborator

@mrawls mrawls left a comment

Choose a reason for hiding this comment

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

Thank you for greatly improving these pipelines across so many cameras! Please ensure they all use two-space indents; I think there are a few files with four space indents.

@kfindeisen
Copy link
Member Author

I thought I managed to be consistent within a single file. If I need to standardize the files, then why two spaces instead of four? I'd find two spaces hard to read, especially for long blocks.

@mrawls
Copy link
Collaborator

mrawls commented Oct 1, 2021

We still need a "real" yaml style guide, don't we... I'm basing my interim 2-space assertion on what DRP.yaml uses and what @jonathansick said on Slack https://lsstc.slack.com/archives/C2JPL2DGD/p1628187106046800.

Copy link
Contributor

@natelust natelust left a comment

Choose a reason for hiding this comment

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

I think this looks good, but in case I missed something I would recommend running pipetask build -p <pipeline.yaml> --show pipeline on each pipeline to make sure they build and produce what you expect

pipelines/LsstCamImSim/ApPipe.yaml Outdated Show resolved Hide resolved
pipelines/LsstCamImSim/ProcessCcd.yaml Outdated Show resolved Hide resolved
pipelines/LsstCamImSim/ProcessCcd.yaml Outdated Show resolved Hide resolved
This commit ensures all instruments have the same pipelines (so that
a user or 3rd-party program need not guess), and has ApPipe and
ApTemplate delegate all single-frame changes to ProcessCcd.
These subsets make it easier to refer to individual pipelines from
within compositions.
This commit does not parameterize ProcessCcd.yaml (which would require
adding a lot of config overrides that are currently absent) nor
DarkEnergyCamera/ApPipe_hits2015-3.yaml (which is supposed to be a
record of a specific pipeline run).
@kfindeisen kfindeisen merged commit 2d4953b into master Oct 8, 2021
@kfindeisen kfindeisen deleted the tickets/DM-26140 branch October 8, 2021 21:18
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

3 participants