Remove optional use of statsmodels within EMOS #1563

gavinevans · 2021-09-22T16:29:58Z

Addresses part of #1537

Description
This PR removes the optional use of statsmodels, meaning that it is now a required module. This aids the further development in #1537 for using a static additional predictor.

Further information in https://github.com/metoppv/mo-blue-team/issues/73#issuecomment-925092996.

Testing:

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

…ow a required module.

codecov · 2021-09-22T16:39:07Z

Codecov Report

Merging #1563 (93a4803) into master (f8e2ce4) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1563      +/-   ##
==========================================
+ Coverage   97.93%   97.97%   +0.03%     
==========================================
  Files         106      107       +1     
  Lines        9463     9562      +99     
==========================================
+ Hits         9268     9368     +100     
+ Misses        195      194       -1

Impacted Files	Coverage Δ
improver/calibration/utilities.py	`98.93% <ø> (+0.95%)`	⬆️
improver/calibration/ensemble_calibration.py	`99.73% <100.00%> (ø)`
improver/threshold.py	`100.00% <0.00%> (ø)`
improver/utilities/rescale.py	`100.00% <0.00%> (ø)`
improver/utilities/temporal.py	`100.00% <0.00%> (ø)`
improver/synthetic_data/set_up_test_cubes.py	`100.00% <0.00%> (ø)`
improver/lightning.py	`100.00% <0.00%> (ø)`
improver/utilities/spatial.py	`98.78% <0.00%> (+0.12%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f8e2ce4...93a4803. Read the comment docs.

tjtg · 2021-09-30T09:18:59Z

I've created a PR-to-PR which changes this to make statsmodels required only for estimate EMOS functionality. That change avoids a hard dependency on statsmodels for all of IMPROVER but still allows for a lot of the simplification in this PR.

I've also had a read through the this PR. All looks OK to me and avoiding two code paths is a nice simplification.

Statsmodels required only for estimate EMOS

fionaRust

A few questions

...er_tests/calibration/ensemble_calibration/test_EstimateCoefficientsForEnsembleCalibration.py

improver_tests/acceptance/SHA256SUMS

fionaRust

Thanks Gavin

MoseleyS

I've got a few questions about this PR, hopefully it is just my lack of understanding.

MoseleyS · 2021-10-04T14:20:04Z

...er_tests/calibration/ensemble_calibration/test_EstimateCoefficientsForEnsembleCalibration.py

@@ -508,6 +484,7 @@ def setUp(self):
        halo surrounding the original data.
        Set up expected results for different situations.
        """
+        pytest.importorskip("statsmodels")


If statsmodels is no longer optional, why are we allowed to skip it?

Thanks @MoseleyS. I think your queries are all related to @tjtg's PR to this PR (gavinevans#13). You're correct that this PR makes statsmodels no longer optional, however, as statsmodels is still only used in one particular place in the codebase, rather than having widespread usage like numpy or Iris, @tjtg has recommended that statsmodels is regarded as an optional module. I think this is a similar set-up to pysteps. Therefore, if someone wants to use components of improver, but isn't interested in EMOS, then they don't need to have statsmodels in their environment.

MoseleyS · 2021-10-05T07:26:58Z

envs/environment_py37_iris30.yml

-  - statsmodels
  # Optional
  - numba
  - pysteps=1.4.1
+  - statsmodels


I think I need this explaining. The statsmodel dependency has moved from "Required" to "Optional", but the PR removes the optional status of this module, which I think makes it "Required".

My previous understanding of "Required" and "Optional" was just like you suggest, which is why I made this PR: #1556. However, the preference, is to treat statsmodels more like pysteps, so even though it is required for EMOS, it is optional for the improver codebase, as someone might want to use improver, but not the EMOS component.

MoseleyS · 2021-10-05T07:27:15Z

envs/environment_py38_iris30.yml

-  - statsmodels
  # Optional
-  - numba
-  - pysteps=1.4.1
-  - timezonefinder=4.1.0


Why are these removed?

There is an interest in representing different things with these different environments with this environment being slightly slimmer. This is the approach chosen by @tjtg in gavinevans#13.

MoseleyS · 2021-10-05T07:27:54Z

improver/calibration/ensemble_calibration.py

@@ -994,6 +993,8 @@ def compute_initial_guess(
            List of coefficients to be used as initial guess.
            Order of coefficients is [alpha, beta, gamma, delta].
        """
+        import statsmodels.api as sm


Why is this import statement not at the top of the file with all the other import statements?

As mentioned above, this is because statsmodels is being treated as optional for the codebase, even though it is required for EMOS. Therefore, if someone isn't using the EMOS component of improver, then they can use a slimmer environment.

MoseleyS · 2021-10-05T09:01:52Z

envs/environment_py38_iris30.yml

@@ -21,11 +21,7 @@ dependencies:
  - scipy=1.6
  - sigtools
  - sphinx
-  - statsmodels


Hi @tjtg. Can we add a comment to this environment stating what is NOT supported, so that we know what to expect. e.g.

# This environment does not support the following CLIs: - estimate-emos-coefficients - generate-timezone-mask-ancillary - nowcast-accumulate - nowcast-extrapolate - nowcast-optical-flow-from-winds - generate-advection-velocities-from-winds

MoseleyS

Approved. I will follow up on a comment in the environment as a separate PR when Tom has had a chance to respond to my question.

* Modifications to remove optional use of statsmodels. statsmodels is now a required module. * Run isort and black. * Flake8 corrections. * Flake8 correction. * Code and test changes to make statsmodels optional * Remove optional libraries from py38 environment * Move statsmodels to optional in py37 environment * Fix black * Re-add warning messages to be ignored. Co-authored-by: Tom Gale <[email protected]>

gavinevans added 4 commits September 22, 2021 17:21

Modifications to remove optional use of statsmodels. statsmodels is n…

cedad9f

…ow a required module.

Run isort and black.

993519a

Flake8 corrections.

a76cc81

Flake8 correction.

93af0d4

gavinevans added the FY21/22 Temperature calibration Owned by Gavin label Sep 22, 2021

gavinevans added this to the 1.0.0 milestone Sep 22, 2021

gavinevans self-assigned this Sep 22, 2021

gavinevans mentioned this pull request Sep 22, 2021

Update EMOS configuration with preferred site temperature calibration approach #1537

Closed

3 tasks

gavinevans mentioned this pull request Sep 23, 2021

Support for a static additional predictor within the EMOS plugins #1564

Merged

2 tasks

tjtg added the BoM review required PRs opened by non-BoM developers that require a BoM review label Sep 27, 2021

tjtg added 4 commits September 30, 2021 18:31

Code and test changes to make statsmodels optional

6fa97fe

Remove optional libraries from py38 environment

9a7d447

Move statsmodels to optional in py37 environment

6417006

Fix black

f3db623

tjtg mentioned this pull request Sep 30, 2021

Statsmodels required only for estimate EMOS gavinevans/improver#13

Merged

Merge pull request #13 from tjtg/optional-statsmodels

e910d92

Statsmodels required only for estimate EMOS

fionaRust reviewed Oct 1, 2021

View reviewed changes

...er_tests/calibration/ensemble_calibration/test_EstimateCoefficientsForEnsembleCalibration.py Show resolved Hide resolved

improver_tests/acceptance/SHA256SUMS Show resolved Hide resolved

Re-add warning messages to be ignored.

93a4803

gavinevans assigned fionaRust and unassigned gavinevans Oct 4, 2021

fionaRust approved these changes Oct 4, 2021

View reviewed changes

fionaRust assigned bayliffe and MoseleyS and unassigned fionaRust and bayliffe Oct 4, 2021

MoseleyS requested changes Oct 5, 2021

View reviewed changes

MoseleyS assigned gavinevans and unassigned MoseleyS Oct 5, 2021

gavinevans assigned MoseleyS and unassigned gavinevans Oct 5, 2021

MoseleyS reviewed Oct 5, 2021

View reviewed changes

MoseleyS approved these changes Oct 6, 2021

View reviewed changes

MoseleyS merged commit 516e9fc into metoppv:master Oct 6, 2021

gavinevans deleted the improver1537_nonoptional_statsmodels branch October 6, 2021 09:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove optional use of statsmodels within EMOS #1563

Remove optional use of statsmodels within EMOS #1563

gavinevans commented Sep 22, 2021 •

edited

Loading

codecov bot commented Sep 22, 2021 •

edited

Loading

tjtg commented Sep 30, 2021 •

edited

Loading

fionaRust left a comment

fionaRust left a comment

MoseleyS left a comment

MoseleyS Oct 4, 2021

gavinevans Oct 5, 2021

MoseleyS Oct 5, 2021

gavinevans Oct 5, 2021

MoseleyS Oct 5, 2021

gavinevans Oct 5, 2021

MoseleyS Oct 5, 2021

gavinevans Oct 5, 2021

MoseleyS Oct 5, 2021

MoseleyS left a comment

Remove optional use of statsmodels within EMOS #1563

Remove optional use of statsmodels within EMOS #1563

Conversation

gavinevans commented Sep 22, 2021 • edited Loading

codecov bot commented Sep 22, 2021 • edited Loading

Codecov Report

tjtg commented Sep 30, 2021 • edited Loading

fionaRust left a comment

Choose a reason for hiding this comment

fionaRust left a comment

Choose a reason for hiding this comment

MoseleyS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MoseleyS left a comment

Choose a reason for hiding this comment

gavinevans commented Sep 22, 2021 •

edited

Loading

codecov bot commented Sep 22, 2021 •

edited

Loading

tjtg commented Sep 30, 2021 •

edited

Loading