Support for a static additional predictor within the EMOS plugins #1564
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #1564 +/- ##
==========================================
+ Coverage 97.98% 98.00% +0.02%
==========================================
Files 107 107
Lines 9619 9669 +50
==========================================
+ Hits 9425 9476 +51
+ Misses 194 193 -1
Continue to review full report at Codecov.
|
gavinevans
force-pushed
the
improver1537_emos_plugins2
branch
from
c931a58 to
7c33f1b
Compare
2 tasks
…misers plugin required to support the use of additional predictors.
…ow a required module.
…atic additional predictor.
…tatic additional predictor.
gavinevans
force-pushed
the
improver1537_emos_plugins2
branch
from
7c33f1b to
544cf4e
Compare
3 tasks
fionaRust
reviewed
Oct 7, 2021
There was a problem hiding this comment.
Thanks for these changes Gavin. My comments are all pretty minor, the main functionality of this code is really good. Two things I've noted:
- I guess there will be another PR at some point to do the CLI changes.
- I think this code will support multiple additional predictors, but all the tests only use one additional predictor. Maybe we should talk through whether we should add a test for multiple additional predictors anywhere in the code.
...over_tests/calibration/ensemble_calibration/test_CalibratedForecastDistributionParameters.py
Outdated
Show resolved
Hide resolved
...over_tests/calibration/ensemble_calibration/test_CalibratedForecastDistributionParameters.py
Outdated
Show resolved
Hide resolved
...er_tests/calibration/ensemble_calibration/test_EstimateCoefficientsForEnsembleCalibration.py
Outdated
Show resolved
Hide resolved
...er_tests/calibration/ensemble_calibration/test_EstimateCoefficientsForEnsembleCalibration.py
Show resolved
Hide resolved
…_plugins2 * upstream/master: Support for a static additional predictor within the CRPS minimisation plugin (metoppv#1575) MOBT-86: Spot extracting from data with 2-dimensional time coordinates (metoppv#1573)
…_plugins2 * upstream/master: Retain ensemble realization numbers from the raw forecast after reordering (metoppv#1580)
2 tasks
…_plugins2 * upstream/master: Increase leniency of EMOS application (metoppv#1577)
MoseleyS
requested changes
Oct 12, 2021
...over_tests/calibration/ensemble_calibration/test_CalibratedForecastDistributionParameters.py
Outdated
Show resolved
Hide resolved
...er_tests/calibration/ensemble_calibration/test_EstimateCoefficientsForEnsembleCalibration.py
Outdated
Show resolved
Hide resolved
...er_tests/calibration/ensemble_calibration/test_EstimateCoefficientsForEnsembleCalibration.py
Outdated
Show resolved
Hide resolved
gavinevans
force-pushed
the
improver1537_emos_plugins2
branch
2 times, most recently
from
dce97d1 to
1215468
Compare
gavinevans
force-pushed
the
improver1537_emos_plugins2
branch
from
1215468 to
c663df5
Compare
MoseleyS
approved these changes
Oct 13, 2021
gavinevans
added a commit
to gavinevans/improver
that referenced
this pull request
Oct 19, 2021
…toppv#1564) * Only the changes required to the ContinuousRankedProbabilityScoreMinimisers plugin required to support the use of additional predictors. * Simplified array reshaping functionality. * Update docstrings. * Edits following review comments. * Correct test name. * Modifications to remove optional use of statsmodels. statsmodels is now a required module. * Run isort and black. * Changes to estimation of EMOS coefficients to support the use of a static additional predictor. * Changes to application of EMOS coefficients to support the use of a static additional predictor. * Update checksums. * Tidy-ups following rebasing. * Modifications following review comments. * Further review amendments. * Minor tidy-ups. * Update checksums. * Edits to revert changes to flatten_ignoring_masked_data and modify additional predictor handling in relation to the initial guess. * Unit test corrections. * Minor corrections. * Update checksums. * Update checksums. * Updates following review comments Co-authored-by: fionaRust <fiona.rust@metoffice.gov.uk>
gavinevans
added a commit
to gavinevans/improver
that referenced
this pull request
Oct 19, 2021
…lar_ingestion_functions * upstream/master: Generate calibrated forecasts from EMOS with an alternative percentile set (metoppv#1587) MOBT-77: Weather symbols to represent an extended period (metoppv#1552) Workaround for slow scipy truncnorm by using the version from 1.3.3 (metoppv#1576) Support for a static additional predictor within the EMOS plugins (metoppv#1564) Fix negative grid spacing (metoppv#1583) Increase leniency of EMOS application (metoppv#1577)
gavinevans
added a commit
to gavinevans/improver
that referenced
this pull request
Oct 19, 2021
…lar_ingestion_cli2 * upstream/master: Generate calibrated forecasts from EMOS with an alternative percentile set (metoppv#1587) MOBT-77: Weather symbols to represent an extended period (metoppv#1552) Workaround for slow scipy truncnorm by using the version from 1.3.3 (metoppv#1576) Support for a static additional predictor within the EMOS plugins (metoppv#1564) Fix negative grid spacing (metoppv#1583) Increase leniency of EMOS application (metoppv#1577)
MoseleyS
pushed a commit
to MoseleyS/improver
that referenced
this pull request
Aug 22, 2024
…toppv#1564) * Only the changes required to the ContinuousRankedProbabilityScoreMinimisers plugin required to support the use of additional predictors. * Simplified array reshaping functionality. * Update docstrings. * Edits following review comments. * Correct test name. * Modifications to remove optional use of statsmodels. statsmodels is now a required module. * Run isort and black. * Changes to estimation of EMOS coefficients to support the use of a static additional predictor. * Changes to application of EMOS coefficients to support the use of a static additional predictor. * Update checksums. * Tidy-ups following rebasing. * Modifications following review comments. * Further review amendments. * Minor tidy-ups. * Update checksums. * Edits to revert changes to flatten_ignoring_masked_data and modify additional predictor handling in relation to the initial guess. * Unit test corrections. * Minor corrections. * Update checksums. * Update checksums. * Updates following review comments Co-authored-by: fionaRust <fiona.rust@metoffice.gov.uk>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #1537
Dependent on #1563 and #1575.
Description
This PR adds support for the use of a static additional predictor within the EMOS plugins. From a metadata perspective, this PR adds the use of a predictor_index and predictor_name coordinates to the beta coefficient cube.
This PR is a slightly slimmed down version of #1555 after creating a standalone PR to deal with environment changes (#1556), a separate PR to remove optional usage of statsmodels (#1563) and a separate PR to include the changes to the CRPS minimisation plugin only. This PR does not include changes to the CLI to support the use of additional predictors, as this is not actually required in the short-term due to the need for #1538. Note that the number of lines changed will reduce by ~300 when #1563 is merged and ~400 when #1575 is merged.
This PR has not provided support for the use of a static additional predictor with the realizations option. This can be revisited, if there is a requirement for this functionality.
Further information is in this comment.
Testing: