Repository navigation
Improve QRF change capping - #2458
Conversation
mo-kbogue
left a comment
mo-kbogue
left a comment
There was a problem hiding this comment.
Thanks for this, great that we can reuse existing functionality rather than needing to add more lines of code, good thinking. Minor suggestions on implementation that are not a necessity. Found the tests a bit difficult to follow, due to not having the uncalibrated and calibrated result in the same place as the quantiles, max_allowed_difference and expected. I understand in general however that this is following the existing convention for these tests and shouldn't be changed. Ran the tests and they all passed.
mo-kbogue
left a comment
mo-kbogue
left a comment
There was a problem hiding this comment.
Just a typo to fix (I think), happy with everything else, thanks for those changes.
| If not provided, no maximum difference check will be applied. Defaults to None. | ||
| The maximum allowed difference between the uncalibrated and calibrated | ||
| forecast. If not provided, no maximum difference check will be applied. | ||
| If provided, must be a non-negative integer. Defaults to None. |
There was a problem hiding this comment.
I think you mean must be a non-negative float? That's what is written below
| If provided, must be a non-negative integer. Defaults to None. | |
| If provided, must be a non-negative float. Defaults to None. |
| assert result.coord("forecast_period").points[0] == 12 * 3600 | ||
|
|
||
|
|
||
| @pytest.mark.parametrize( |
There was a problem hiding this comment.
For me, this could do with a comment explaining what the forecast values are, and what the reference forecast values are - so it's easier to reason about the expected output
jdjonathandavies
left a comment
jdjonathandavies
left a comment
There was a problem hiding this comment.
This looks great - especially liked the removal of the now unneeded code from the QRF plugin.
Happy to approve, although I had a minor comment that would help the readability of the tests.
…ve defunct unit tests of ApplyQuantileRegressionRandomForests. Fix mistake when deleting sections of ApplyQuantileRegressionRandomForests. Fix typo.
… plugin. Add unit tests for this validation. Fix typo in unit test description.
4f0e7a0 to
8c2175b
Compare
Addresses https://github.com/metoppv/mo-blue-team/issues/1292
Modifies how QRF calibrated forecasts are clipped to be within some range of the uncalibrated forecast. Previously, the clipping used bounds which were a function of the maximum and minimum of the uncalibrated forecast at each site, however this meant that calibrated forecasts which were entirely outside of this range would be clipped so that all of their percentiles were equal. The new approach applies per-quantile bounds when clipping the calibrated forecast.
This PR removes the old implementation and unit tests in favour of a new approach utilising the EnforceConsistentForecasts plugin. New unit tests are added for this approach.
Testing: