Skip to content

Add constructor to QualityMetric ABC class - #258

Merged
roryclaydon1994 merged 4 commits into
developfrom
257-add-constructor-to-qualitymetric-abc-class
Sep 18, 2026
Merged

roryclaydon1994 merged 4 commits into
developfrom
257-add-constructor-to-qualitymetric-abc-class

Conversation

@jeipollack

@jeipollack jeipollack commented Sep 16, 2026 •

Copy link
Copy Markdown
Contributor

Summary

This PR adds a missing step in the quality control pipeline: passing metric configuration parameters to quality metric implementations.

Closes #257

What’s changed

  • Add a constructor to QualityMetric to initialise the metric parameters attribute
  • Update QualityControlPipeline._initialise_metrics() to pass metric configuration parameters to metric class constructors
  • Add type hints to QualityMetric and its concrete implementations
  • Replace wrong object QualityMetric.compute(dataset) with QualityMetric.compute(context)
  • Define the return type of QualityMetric.compute() as dict[str, np.ndarray] to support multiple named diagnostic results from a metric

How to test / verify

  • No tests were updated.
  • CI passes.

Scope

Indicate the type of PR:

  • Feature
  • Bug fix
  • Hotfix
  • Documentation / process change
  • Internal / refactor
  • Release

Optionally, note if this PR is part of a larger milestone or set of related PRs.

Changelog

Did this PR introduce user-visible changes?
If yes, a Scriv changelog fragment must be added and committed.

  • Changelog fragment added (if applicable)

Reviewer Checklist

Reviewers should confirm the following before approving and merging:

  • The PR targets the correct base branch (develop, or main for release PRs)
  • The PR is assigned to the developer
  • Appropriate labels are applied
  • The PR is included in relevant projects and/or milestones
  • Description clearly explains what has changed
  • Issue references included, if applicable
  • Code and documentation adhere to current standards (ruff)
  • Documentation updates included, if relevant
  • CI tests are passing
  • All reviewer comments have been addressed

Next Steps / Notes (if applicable)

The design of PixelMaskMetrics (#254) calls for four diagnostic metrics to be returned as a dictionary by PixelMaskMetrics.compute(). This motivated the change to the QualityMetric.compute() return type to support multiple named diagnostic results. A rejection policy can then select which diagnostic(s) it uses in its filtering strategy.

The next step is to proceed with the implementation of PixelMaskMetrics.

- Add constructor to initialise the metric parameters attribute
- Update QualityControlPipeline._initialise_metrics to pass
  metric configuration parameters to metric class constructors
- Add type hints in QualityMetric and its concrete implementation
Jennifer Pollack added 2 commits September 18, 2026 14:00
- Replace dataset with QualityControlContext
- Update doc string
- Replace dataset: Any with context: QualityControlContext
- Update imports

@roryclaydon1994 roryclaydon1994 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review Feedback

PR adds to the API for the QualityMetric ABC to allow customisation of the metrics parameters and also collects resources and datasets into a context which helps keeps everything together. Only three small questions on this.

Comment thread src/wf_psf/quality_control/metrics/base.py
Comment thread src/wf_psf/quality_control/metrics/base.py Outdated
Comment thread src/wf_psf/quality_control/metrics/base.py
@roryclaydon1994
roryclaydon1994 merged commit 736b7e4 into develop Sep 18, 2026
2 checks passed
@roryclaydon1994
roryclaydon1994 deleted the 257-add-constructor-to-qualitymetric-abc-class branch September 18, 2026 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Development

Successfully merging this pull request may close these issues.

2 participants