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-39214: Move ingredient pipelines into _ingredients dir #186

Merged
merged 1 commit into from May 31, 2023

Conversation

leeskelvin
Copy link
Contributor

No description provided.

@leeskelvin leeskelvin force-pushed the tickets/DM-39214 branch 8 times, most recently from 0581a88 to 5c10a26 Compare May 30, 2023 19:27
Copy link
Member

@kfindeisen kfindeisen left a comment

Choose a reason for hiding this comment

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

I'm a bit concerned about applying the boilerplate to some of ap_verify's weirdness, but otherwise looks good.

This directory contains pipeline definition YAML files which are used when processing data with the LSST Science Pipelines.

The pipelines defined here come in three flavors: camera-specific (within named directories), camera-agnostic (top-level, if any), and building-block ingredients (within the [\_ingredients](_ingredients) directory).
Pipelines within the ingredients directory tend to be imported by other pipelines, and are not intended to be used directly by end-users for science purposes.
Copy link
Member

Choose a reason for hiding this comment

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

The original description sounds a bit odd, as if you were documenting a natural phenomenon:

Suggested change
Pipelines within the ingredients directory tend to be imported by other pipelines, and are not intended to be used directly by end-users for science purposes.
Pipelines within the ingredients directory are meant to be imported by other pipelines, and are not intended to be used directly by end-users for science purposes.

pipelines/_ingredients/ApVerify.yaml Show resolved Hide resolved
pipelines/README.md Show resolved Hide resolved
This directory contains pipeline definition YAML files which are used when processing data with the LSST Science Pipelines.

The pipelines defined here come in three flavors: camera-specific (within named directories), camera-agnostic (top-level, if any), and building-block ingredients (within the [\_ingredients](_ingredients) directory).
Pipelines within the ingredients directory tend to be imported by other pipelines, and are not intended to be used directly by end-users for science purposes.
Copy link
Member

Choose a reason for hiding this comment

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

Nothing in ap_verify should be used for science purposes, something I occasionally butt heads with users about. Perhaps:

Suggested change
Pipelines within the ingredients directory tend to be imported by other pipelines, and are not intended to be used directly by end-users for science purposes.
Pipelines within the ingredients directory tend to be imported by other pipelines, and are not intended to be used directly by end-users for verification purposes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This may be a matter of definition - isn't verification a part of the scientific process? If this is a point of contention then we could just remove the latter part of this sentence, ending with "by end-users."

Copy link
Member

Choose a reason for hiding this comment

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

When I hear a phrase like "science runs" or "scientific processing", it usually means running a pipeline on a generic set of data. The ap_verify pipelines, unlike the ap_pipe pipelines, are not designed to be run on anything other than a packaged ap_verify dataset -- it might work, but it's very Not Supported.

I'm fine with just removing the final phrase.

Comment on lines 5 to 8
The pipelines defined here are intended to be used as building blocks for more complex pipelines.
As such, they are not intended to be used directly by end-users for science purposes.

Science ready pipeline definitions are located in the [pipelines](..) directory.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The pipelines defined here are intended to be used as building blocks for more complex pipelines.
As such, they are not intended to be used directly by end-users for science purposes.
Science ready pipeline definitions are located in the [pipelines](..) directory.
The pipelines defined here are intended to be used as building blocks for more complex pipelines.
As such, they are not intended to be used directly by end-users for verification purposes.
Standalone pipeline definitions are located in the [pipelines](..) directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See above for discussion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Modifying this sentence to end with "by end-users.", as discussed above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replacing "Standalone" with "User-facing". The term standalone to me implies that these may be used in isolation, but more often than not they import a series of other pipeline YAMLs in order to function correctly.

@leeskelvin leeskelvin merged commit 8bfe6ca into main May 31, 2023
1 check passed
@leeskelvin leeskelvin deleted the tickets/DM-39214 branch May 31, 2023 14:59
kfindeisen added a commit that referenced this pull request Aug 2, 2023
These changes were overlooked on #186.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants