Repository navigation
260 Update QualityControlPipeline to consume the QualityMetric.compute() dictionary-based API - #261
jeipollack wants to merge 7 commits into
Conversation
- Add diagnostic attribute to RejectionPolicyConfig - Update rejection config section parser - Remove validation methods added to pipeline.py - Update config_test.py with new and improved test cases - Update fixtures and remove deprecated YAML fixtures
- Add validation for metric resource requirements and rejection policy configuration - Call configuration validation from the pipeline constructor - Update pipeline execution to consume the `QualityMetric.compute` diagnostics API - Apply rejection policies using their configured diagnostic - Move cross-section validation tests from config_test.py to pipeline_test.py - Update and extend pipeline test coverage for the revised validation and updated processing flow
jeipollack
added
bug
roryclaydon1994
left a comment
roryclaydon1994
left a comment
There was a problem hiding this comment.
Review Feedback
There were a lot of changes, so I might have missed something, but overall design makes sense.
I couldn't see an example of applying multiple rejection policies per quality metric. Is that an expected use case?
| """ | ||
|
|
||
| enabled: bool = False | ||
| diagnostic: str | None = None |
There was a problem hiding this comment.
Would it ever be valid to allow a RejectionPolicyConfig to be created with a diagnostic=None? I'm wondering if this should be a required parameter
There was a problem hiding this comment.
if it's None, then just quality metrics computation part of the pipeline runs.
There was a problem hiding this comment.
I should have waited to reply when I could really concentrate. I agree with your suggestion. Metric evaluation without implementing a rejection policy is already covered by the enabled flag. I'll make diagnostic required so that each configured rejection policy explicitly identifies the metric diagnostic it evaluates.
| validity_masks[name] = policy.apply(diagnostic) | ||
|
|
||
| if validity_masks: | ||
| if validity_masks != {}: |
There was a problem hiding this comment.
Just curious what you found that required the comparison to an empty dict?
There was a problem hiding this comment.
if diagnostic is None, then rejection policy application part of the pipeline will not run only the quality metrics. I think Quality Control can also include just computing some metrics without rejecting samples. does that make sense?
There was a problem hiding this comment.
Is it to allow an explicit None to be allowed through the guard?
There was a problem hiding this comment.
My previous comment is no longer relevant. The loop that generates validity masks only iterates over enabled rejection policies. If no policies are enabled, _instantiate_rejection_policies() returns an empty dictionary, so validity_masks remains empty.
The integration test test_pipeline_run_rejection_policy_disabled confirms that the metrics are still computed, but rejection policies are not applied when disabled, even when their diagnostic parameters are valid. In this case, the pipeline returns an all-True validity mask.
There was a problem hiding this comment.
is this fine for you?
There was a problem hiding this comment.
I would say if
if validity_masks:
works then we go for that as it's more pythonic and it would prevent a None value being passed on.
|
Thanks @roryclaydon1994 for your careful review! I believe I have addressed each comment and query. I am ready for you to take a second look. |
Summary
This PR updates the pipeline and configuration handling to support the dictionary-based metric API, allowing a rejection policy to select a specific diagnostic produced by a quality metric.
Context: There was an incompatibility between the
QualityMetric.compute()API introduced in PR #258 and the quality-control pipeline, which previously assumed that a metric returned a single array rather than multiple diagnostics in a dictionary.closes #260
Several files were modified in this PR to resolve the bug, but all are within the scope of the changes. I recommend taking a look at the referenced issue and the Notes section at the bottom, where I tried to provide some additional context on the changes and where to start the review. If you have questions about the processing flow, I'm happy to set up a meeting to go through it together.
What’s changed
QualityControlPipelineto consume theQualityMetric.compute()dictionary-based API.QualityMetric.diagnosticsclass attribute.diagnosticfield toRejectionPolicyConfigto select which metric diagnostic is used by a rejection policy.config_test.pytopipeline_test.py.How to test / verify
config_test.py, particularly parsing and validation of rejection-policy configuration.pipeline_test.pyfor configuration validation and the updated metric → diagnostic → rejection-policy processing flow.Scope
This PR is part of the quality-control/outlier-rejection work.
Changelog
The changes are internal and do not introduce a user-visible change to the public product behaviour.
Reviewer Checklist
develop, ormainfor release PRs)ruff)Next Steps / Notes (if applicable)
The recommended review path is to start with
quality_control/pipeline.pyto understand the end-to-end processing flow, then trace the corresponding changes intoconfig.py,metrics/base.py, the concrete metrics, and the tests.The main processing flow is:
configuration → pipeline → resource resolution → metric computation → diagnostics → rejection policy → validity masks → QualityControlResultThe associated issue contains the more detailed architectural discussion and validation results.