Add json file input to threshold interpolation #2114

maxwhitemet · 2025-03-27T15:53:28Z

Addresses #866

Description

Updated plugin and CLI to accept a JSON file
Associated JSON file for tests available here

Testing:

Ran tests and they passed OK
Added new tests for the new feature(s)

codecov · 2025-03-27T16:08:00Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.46%. Comparing base (84a8944) to head (6b6d2bf).
⚠️ Report is 156 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2114      +/-   ##
==========================================
+ Coverage   98.39%   98.46%   +0.07%     
==========================================
  Files         124      139      +15     
  Lines       12212    13777    +1565     
==========================================
+ Hits        12016    13566    +1550     
- Misses        196      211      +15

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

brhooper

Thanks @maxwhitemet. I've added some suggestions, the mains ones being around changing the format of the json file to simplify the unnecessary "threshold: None" format.

improver/utilities/threshold_interpolation.py

improver/cli/threshold_interpolation.py

improver/utilities/threshold_interpolation.py

improver_tests/acceptance/test_threshold_interpolation.py

improver_tests/utilities/test_ThresholdInterpolation.py

maxwhitemet · 2025-04-07T14:41:28Z

Thank you for the review @brhooper. I have implemented all of the suggested changes.

brhooper

Thanks @maxwhitemet, I've added some comments.

improver/cli/threshold_interpolation.py

brhooper · 2025-04-08T06:48:41Z

improver/utilities/threshold_interpolation.py

        self,
        threshold_values: Optional[List[float]] = None,
-        threshold_config: Optional[Dict[str, Union[List[float], str]]] = None,
+        threshold_config: Optional[Union[List[float], Dict[str, str]]] = None,


Suggested change

threshold_config: Optional[Union[List[float], Dict[str, str]]] = None,

threshold_config: Optional[Union[List[float]], Dict[str, Union[List[float], str]]] = None,

I can't work out why this was changed, so if I've missed something then please let me know. I think the typing only needed the addition of Union[List[float]], ... though.

Original annotation
The original annotation comes from the threshold.py plugin and suggests dictionary values can be either a list of fuzzy bound floats or a "None" string.

Annotation I suggested
My annotation suggests that if a dictionary is provided, the dictionary's values can only be a "None" string. We don't use fuzzy bounds for threshold interpolation.

I have kept my original change for now. Please let me know if I have misunderstood something

brhooper · 2025-04-08T06:50:25Z

improver/utilities/threshold_interpolation.py

-        threshold_values: Optional[Union[float, List[float]]],
-        threshold_config: Optional[dict],
+        threshold_values: Optional[List[float]] = None,
+        threshold_config: Optional[Union[List[float], Dict[str, str]]] = None,


Suggested change

threshold_config: Optional[Union[List[float], Dict[str, str]]] = None,

threshold_config: Optional[Union[List[float]], Dict[str, Union[List[float], str]]] = None,

Same suggestion as above - sorry if I've missed something.

improver/utilities/threshold_interpolation.py

improver_tests/utilities/test_ThresholdInterpolation.py

Kat-90

Thanks Max.
Your PR description needs fleshing out a bit as you've changed more than just adding a JSON input now.

My tests fail the checksums as well so you might need to update them and repush.
I've left a comment on your test_data PR as well: #2114

improver/utilities/threshold_interpolation.py

improver_tests/utilities/test_ThresholdInterpolation.py

cpelley · 2025-05-28T08:29:19Z

Looking at your type hints, seems to me that the improvers json input functionality exposes its limited deserialisation capability (internal technical implementation detail) to functions that need to use it.

I would expect null to be used within a json to allow built-in deserialisation to a Python None and keys that represent a string of floats or ints to be converted appropriately by improvers deserialisation capability.

json containing:

{
    "1.5": [2.0, 3.0],
    "2.0": null
}

These input dictionaries to these functions should be considered dictionaries containing the expected types, not those representing types characterising the spec of json.

I would expect the above json to be deserialised appropriately to the following python dictionary:

{
    1.5: [2.0, 3.0],
    2.0: None
}

In future, we will want to be removing the clize layer. I don't think it ideal for functions to individually handle/be exposed the deserialisation limitations of improver json loading. This unnecessarily binds the two layers together.

Not a blocker for this PR of course, this wasn't due to your changes, I just noticed as your changes drew attention to this issue.

Note: the above is conjecture right now, purely from reasoning your type hints, not by looking at improver json input loading capability or testing it.

…ak anything in pre-written tests

* Changed plugin's functionality when new thresholds have different units such that the output cube's thresholds are converted back to the units of the input cube.

…imension from also modifying the cube's overall units. * Implemeneted new variables in the cli call and created appropriate tests

- More clearly documented ability to supply only a single numeric value for threhsold_values - Added a unit test for when only a single numeric value is provided for threshold_values - Fixed typos

Kat-90

When reviewing the suite ticket I noticed there is no redundancy for passing a config file into threshold_values, or a value list into threshold_config. This should probably be handled here or we should change the inputs to just accept "thresholds" for which the format is then handled within the plugin itself and the user doesn't need to choose which they think they should use.

github-actions · 2025-07-30T02:10:24Z

In order to maintain a backlog of relevant PRs, we automatically label them as stale after 60 days of inactivity.

If this PR is still important to you, then please comment on this PR and the stale label will be removed.

Otherwise this PR will be automatically closed in 30 days time.

github-actions · 2025-10-07T01:45:22Z

In order to maintain a backlog of relevant PRs, we automatically label them as stale after 60 days of inactivity.

If this PR is still important to you, then please comment on this PR and the stale label will be removed.

Otherwise this PR will be automatically closed in 30 days time.

github-actions · 2025-12-14T02:08:39Z

In order to maintain a backlog of relevant PRs, we automatically label them as stale after 60 days of inactivity.

If this PR is still important to you, then please comment on this PR and the stale label will be removed.

Otherwise this PR will be automatically closed in 30 days time.

maxwhitemet requested a review from brhooper March 27, 2025 15:53

maxwhitemet assigned brhooper Mar 28, 2025

brhooper requested changes Apr 4, 2025

View reviewed changes

brhooper assigned maxwhitemet and unassigned brhooper Apr 4, 2025

maxwhitemet assigned brhooper and unassigned maxwhitemet Apr 7, 2025

brhooper requested changes Apr 8, 2025

View reviewed changes

brhooper assigned maxwhitemet and unassigned brhooper Apr 8, 2025

maxwhitemet assigned maxwhitemet and Kat-90 and unassigned maxwhitemet Apr 11, 2025

Kat-90 added the Paused label Apr 23, 2025

Kat-90 requested changes May 28, 2025

View reviewed changes

bayliffe assigned maxwhitemet and unassigned Kat-90 May 28, 2025

bayliffe removed the Paused label May 28, 2025

maxwhitemet added 8 commits May 28, 2025 12:44

Adjusted threshold_interpolation.py to accept JSON input. Doesn't bre…

ecad9e7

…ak anything in pre-written tests

Implemented unit tests for threshold_units input

05f9aaa

* Implemented unit tests for JSON file input

6fedf25

* Changed plugin's functionality when new thresholds have different units such that the output cube's thresholds are converted back to the units of the input cube.

Removed uncalled variable

39d9c0b

Removed redundant print statements

dcb4b28

* Updated functionality to prevent unit conversion of the threshold d…

80756b6

…imension from also modifying the cube's overall units. * Implemeneted new variables in the cli call and created appropriate tests

Recreated checksums

7124375

new checksums to accompany new test data

1202498

maxwhitemet added 8 commits May 28, 2025 12:44

improver_tests/acceptance/SHA256SUMS

ce25ac6

Updated checksums for new acceptance test

c94d144

Implemented feedback and added JSON list input option

f203ef7

Renamed JSON acceptance test functions

4b23a3d

Resolved unsafe fixes in pre-commit check

e42a347

Implemented suggested changes

4f99b37

Implemented requested changes from review

22b752a

- More clearly documented ability to supply only a single numeric value for threhsold_values - Added a unit test for when only a single numeric value is provided for threshold_values - Fixed typos

Resolved pre-commit failures

6b6d2bf

maxwhitemet force-pushed the add_JSON_file_input_to_threshold_interpolation branch from 7587361 to 6b6d2bf Compare May 28, 2025 12:05

maxwhitemet assigned Kat-90 and unassigned maxwhitemet May 28, 2025

Kat-90 requested changes May 30, 2025

View reviewed changes

Kat-90 marked this pull request as draft May 30, 2025 09:04

github-actions bot added the Stale label Jul 30, 2025

maxwhitemet added Paused and removed Stale labels Jul 30, 2025

cpelley mentioned this pull request Aug 7, 2025

json deserialisation capabiility decoupling from individual appplications #2175

Open

github-actions bot added Stale and removed Stale labels Oct 7, 2025

github-actions bot added the Stale label Dec 14, 2025

	threshold_config: Optional[Union[List[float], Dict[str, str]]] = None,
	threshold_config: Optional[Union[List[float]], Dict[str, Union[List[float], str]]] = None,

Add json file input to threshold interpolation #2114

Are you sure you want to change the base?

Add json file input to threshold interpolation #2114

Uh oh!

Conversation

maxwhitemet commented Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

brhooper left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maxwhitemet commented Apr 7, 2025

Uh oh!

brhooper left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

brhooper Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

maxwhitemet Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

brhooper Apr 8, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Kat-90 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cpelley commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kat-90 left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jul 30, 2025

Uh oh!

github-actions bot commented Oct 7, 2025

Uh oh!

github-actions bot commented Dec 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

maxwhitemet commented Mar 27, 2025 •

edited

Loading

codecov bot commented Mar 27, 2025 •

edited

Loading

cpelley commented May 28, 2025 •

edited

Loading