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-43077: Convert all tasks to use CalibrateImageTask outputs #236

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

parejkoj
Copy link
Contributor

Remove now unused datasets.

{Summary of changes. Prefix PR title with JIRA issue.}


  • Do unit tests pass (scons and/or stack-os-matrix)?
  • Did you run ap_verify.py on at least one of the standard datasets?
    For changes to metrics, the print_metricvalues script from lsst.verify will be useful.
  • Is the Sphinx documentation up-to-date?

@@ -35,7 +35,6 @@ tasks:
class: lsst.pipe.tasks.postprocess.ConsolidateVisitSummaryTask
config:
connections.calexpType: parameters.fakesType
connections.calexp: "{calexpType}initial_pvi"
Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure this one won't work, because the default is initial_pvi without referencing calexpType. Can you confirm that the fakes pipeline has the correct structure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. calibrateImage isn't setup to work with the fakes pipeline as it is used in AP, I don't think. This may require some discussion: I put it on the schedule for Monday's meeting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On second thought, maybe it's fine: ci_ap passed on Jenkins, and that runs the fakes pipeline for DC2.

Copy link
Member

Choose a reason for hiding this comment

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

Just for the record, this question cross-references lsst/pipe_tasks#979.

Copy link
Member

Choose a reason for hiding this comment

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

I've confirmed that the pipeline is incorrect; the task converts initial_pvi into fakes_visitSummary. Please fix (and in the future, please do this check yourself).

ApVerify

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.

The PR changes the pipeline structure by replacing a fakes input with a non-fakes input.

@@ -35,7 +35,6 @@ tasks:
class: lsst.pipe.tasks.postprocess.ConsolidateVisitSummaryTask
config:
connections.calexpType: parameters.fakesType
connections.calexp: "{calexpType}initial_pvi"
Copy link
Member

Choose a reason for hiding this comment

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

I've confirmed that the pipeline is incorrect; the task converts initial_pvi into fakes_visitSummary. Please fix (and in the future, please do this check yourself).

ApVerify

Remove now unused datasets.
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