Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Mobt 496 enforce forecast consistency #1900

Merged
merged 14 commits into from
May 26, 2023
Merged

Mobt 496 enforce forecast consistency #1900

merged 14 commits into from
May 26, 2023

Conversation

brhooper
Copy link
Contributor

@brhooper brhooper commented Apr 28, 2023 •
edited
Loading

Addresses https://github.com/metoppv/mo-blue-team/issues/496

Creates a plugin and corresponding CLI which enables the general enforcement of consistency between a forecast cube and a linear transformation of a reference cube. This plugin specifically includes the functionality to enforce consistency between cloud amount probability forecasts (here), enforcing consistency between wind gust and wind speed percentile forecasts (here), and should also provide a general solution to requirements for enforcing consistency between forecast cubes when future use-cases arise.

The previous specific implementation of a plugin to enforce consistency between cloud amount forecasts is removed.

Acceptance test data: metoppv/improver_test_data#6

Description

Testing:

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

@brhooper brhooper mentioned this pull request Apr 28, 2023
Merged
@codecov
Copy link

codecov bot commented Apr 28, 2023 •
edited
Loading

Codecov Report

Merging #1900 (834ea2a) into master (c2df37b) will increase coverage by 1.33%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1900      +/-   ##
==========================================
+ Coverage   96.99%   98.32%   +1.33%     
==========================================
  Files         119      120       +1     
  Lines       11329    11376      +47     
==========================================
+ Hits        10989    11186     +197     
+ Misses        340      190     -150
Impacted Files Coverage Δ
improver/calibration/reliability_calibration.py 98.94% <ø> (-0.05%) ⬇️
improver/utilities/enforce_consistency.py 100.00% <100.00%> (ø)

... and 10 files with indirect coverage changes

@mspelman07 mspelman07 added this to the IM2023.1 milestone May 2, 2023
@Katie-Howard Katie-Howard self-requested a review May 3, 2023 09:27
Katie-Howard
Katie-Howard requested changes May 4, 2023
View reviewed changes
Copy link
Contributor

@Katie-Howard Katie-Howard left a comment

Choose a reason for hiding this comment

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

I have looked through all your changes, and they generally look sensible and like they do what they are supposed to 👍 I just have a few minor comments for you to look at.

I have run the unit and acceptance tests, and they all run fine, apart from one. There was one failure in improver_tests/test_source_code.py in test_init_files_exist(), where there was an AssertionError.

improver_tests/acceptance/test_enforce_consistent_forecasts.py Show resolved Hide resolved
improver_tests/utilities/test_enforce_consistency.py Outdated Show resolved Hide resolved
@brhooper
Copy link
Contributor Author

brhooper commented May 4, 2023

Thanks for your review @Katie-Howard. I've responded to your 2 review comments.

I haven't been able to reproduce the failure of improver_tests/test_source_code.py in test_init_files_exist() which you have been experiencing. Is it possible it could be an issue at your end?

@brhooper brhooper assigned Katie-Howard and unassigned brhooper May 4, 2023
Katie-Howard
Katie-Howard previously approved these changes May 4, 2023
View reviewed changes
Copy link
Contributor

@Katie-Howard Katie-Howard left a comment

Choose a reason for hiding this comment

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

I've looked through the comments and changes and I'm happy with this. If the unit tests pass for you, it must be something on my system.

@Katie-Howard Katie-Howard removed their assignment May 4, 2023
gavinevans
gavinevans requested changes May 5, 2023
View reviewed changes
Copy link
Contributor

@gavinevans gavinevans left a comment

Choose a reason for hiding this comment

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

Thanks @brhooper 👍

I've added some very minor comments.

improver/utilities/enforce_consistency.py Outdated Show resolved Hide resolved
improver/utilities/enforce_consistency.py Outdated Show resolved Hide resolved
improver_tests/acceptance/SHA256SUMS Outdated Show resolved Hide resolved
improver_tests/utilities/test_enforce_consistency.py Outdated Show resolved Hide resolved
improver/cli/enforce_consistent_forecasts.py Outdated Show resolved Hide resolved
@gavinevans gavinevans assigned brhooper and unassigned gavinevans May 5, 2023
gavinevans
gavinevans requested changes May 5, 2023
View reviewed changes
Copy link
Contributor

@gavinevans gavinevans left a comment

Choose a reason for hiding this comment

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

Thanks @brhooper 👍

I've added a few queries.

improver/utilities/enforce_consistency.py Outdated Show resolved Hide resolved
improver_tests/utilities/test_enforce_consistency.py Outdated Show resolved Hide resolved
improver/utilities/enforce_consistency.py Show resolved Hide resolved
improver/cli/enforce_consistent_forecasts.py Outdated Show resolved Hide resolved
@brhooper brhooper assigned gavinevans and unassigned brhooper May 11, 2023
@brhooper
Update docstrings to include applications to realization data. Add un…
a5eef62 
…it and acceptance tests for realization data. Update checksum for new acceptance test data.
@brhooper brhooper assigned brhooper and unassigned gavinevans May 12, 2023
@brhooper brhooper assigned brhooper and gavinevans and unassigned brhooper May 15, 2023
gavinevans
gavinevans requested changes May 15, 2023
View reviewed changes
Copy link
Contributor

@gavinevans gavinevans left a comment

Choose a reason for hiding this comment

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

Thanks @brhooper 👍

I've added a few minor comments.

improver/cli/enforce_consistent_forecasts.py Outdated Show resolved Hide resolved
improver/utilities/enforce_consistency.py Outdated Show resolved Hide resolved
improver_tests/utilities/test_enforce_consistency.py Outdated Show resolved Hide resolved
improver/cli/enforce_consistent_forecasts.py Show resolved Hide resolved
improver_tests/utilities/test_enforce_consistency.py Outdated Show resolved Hide resolved
gavinevans
gavinevans requested changes May 15, 2023
View reviewed changes
Copy link
Contributor

@gavinevans gavinevans left a comment

Choose a reason for hiding this comment

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

Thanks @brhooper 👍

I've added a few minor comments.

@gavinevans
Copy link
Contributor

This PR now seems fine to me and can be merged when an associated suite change is made.

@brhooper brhooper assigned brhooper and unassigned gavinevans May 16, 2023
@gavinevans gavinevans merged commit 597b578 into metoppv:master May 26, 2023
@brhooper brhooper mentioned this pull request Jun 2, 2023
Merged
2 tasks
MoseleyS pushed a commit to MoseleyS/improver that referenced this pull request Aug 22, 2024
@brhooper
Mobt 496 enforce forecast consistency (metoppv#1900)
29501ab 
* Remove obsolete enforce_consistent_probabilities code. Update checksums

* Add plugin and CLI for general enforcement of forecast consistency. Add corresponding unit and acceptance tests.

* Fix error in acceptance test.

* Corrections to formatting.

* Remove obsolete module import.

* Update docstrings.

* Minor change following review.

* Minor changes following review

* Updated checksums following reduction in acceptance test data size

* Update docstrings to include applications to realization data. Add unit and acceptance tests for realization data. Update checksum for new acceptance test data.

* Add limitation that only the identity function can be applied to probability reference forecasts.

* Formatting corrections

* Minor changes following review. Update checksums for change to acceptance test data.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gavinevans gavinevans gavinevans approved these changes

@Katie-Howard Katie-Howard Katie-Howard left review comments

Assignees

@brhooper brhooper

Labels
None yet
Projects
None yet
Milestone
IM2023.1
Development

Successfully merging this pull request may close these issues.
4 participants
@brhooper @gavinevans @Katie-Howard @mspelman07