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-42419: Break up ApVerify into pure with- and without-fakes pipelines #213

Merged
merged 3 commits into from Feb 23, 2024

Conversation

kfindeisen
Copy link
Member

This PR removes all unnecessary without-fakes processing from ApVerifyWithFakes, and moves all metrics that are better measured without fakes to ApVerify. With no more need to disambiguate, it also removes the WithFakes suffix from individual tasks in ApVerifyWithFakes.


  • 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?

Copy link
Contributor

@BrunoSanchez BrunoSanchez left a comment

Choose a reason for hiding this comment

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

Looks perfect.
Maybe the comments for clarifying would be useful for future, but probably not needed.

pipelines/_ingredients/ApVerifyWithFakes.yaml Show resolved Hide resolved
pipelines/_ingredients/MetricsRuntime.yaml Show resolved Hide resolved
Copy link
Contributor

@BrunoSanchez BrunoSanchez left a comment

Choose a reason for hiding this comment

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

Looks perfect.
Maybe the comments for clarifying would be useful for future, but probably not needed.

Timing is problematic in ApVerifyWithFakes, because the fakes add more
work to catalog-type tasks, and with-fakes and no-fakes tasks cannot
be usefully compared. Source count metrics are, of course, directly
affected by fakes.
This change linearizes the pipeline at the cost of delegating all
non-fakes metric to ApVerify.yaml.
Now that with- and without-fakes processing is cleanly separated,
there's no chance of confusion as to which task is which.
@kfindeisen kfindeisen merged commit e6e1bc7 into main Feb 23, 2024
2 checks passed
@kfindeisen kfindeisen deleted the tickets/DM-42419 branch February 23, 2024 21:42
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