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-39227: rename (with deprecation) SubtractImages external-calibration connections. #270

Merged
merged 3 commits into from Jul 13, 2023

Conversation

TallJimbo
Copy link
Member

No description provided.

Copy link
Contributor

@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.

Looks good, but please settle on using either calibrationsCatalog or visitSummary consistently throughout.

@@ -69,12 +72,26 @@ class SubtractInputConnections(lsst.pipe.base.PipelineTaskConnections,
dimensions=("instrument", "visit"),
storageClass="ExposureCatalog",
name="finalVisitSummary",
# TODO: remove on DM-39854.
deprecated=(
"Deprecated in favor of visitSummary. Will be removed after v26."
Copy link
Contributor

Choose a reason for hiding this comment

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

After v27?

Copy link
Member Author

Choose a reason for hiding this comment

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

When I wrote this, yes, it should have been v27. Now that RFC-945 is adopted I'm going to anticipate its implementation by just a bit and leave it as v26.

"Deprecated in favor of visitSummary. Will be removed after v26."
)
)
visitSummary = connectionTypes.Input(
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency, please either name this calibrationsCatalog in the connections, or use visitSummary later instead of calibrationsCatalog

A few of these were above our docstring column limit, but most just
had line breaks in arbitrary places.  Automatic docstring-wrapping
tools are great!
The new names reflect the possibility of getting PhotoCalibs and WCSs
from the same catalogs, though we're not doing that yet.

In addition, whether external calibrations are applied when 'run'
methods are called directly is now contingent on whether external
calibrations were passed in, not a config option; the config option
just controls whether those connections are active (and hence how
'run' is called) when being run as a PipelineTask.
@TallJimbo TallJimbo merged commit ae19805 into main Jul 13, 2023
2 checks passed
@TallJimbo TallJimbo deleted the tickets/DM-39227 branch July 13, 2023 14:41
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

2 participants