Update tests for multimodel statistics preprocessor #1023

stefsmeets · 2021-02-25T15:51:34Z

Description

Hi everyone, we are developing a new implementation of multi-model statistics preprocessor in #968. The goal of this PR is to to better chart out the requirements for the preprocessor, and to have a more complete description of the functionality through the unit tests, so that no functionality / features are lost.

The current set of tests does not cover all the functionality and is difficult to extend as it still makes use of the old unittest framework. This PR re-implements the functionality of the old tests using pytest.

Closes #issue_number
Link to documentation:

Before you get started

☝ Create an issue to discuss what you are going to do

Checklist

PR has a descriptive title for the changelog
Labels are assigned so they can be used in the changelog
Code follows the style guide
Documentation is available for new functionality
YAML files pass pre-commit or yamllint checks
Circle/CI tests pass
Codacy code quality checks pass
Documentation builds successfully on readthedocs
Unit tests are available

To help with the number pull requests:

🙏 We kindly ask you to review two other open pull requests in this repository

stefsmeets · 2021-02-25T16:08:38Z

One test that is failing (marked with xfail) is related to passing cubes with non-overlapping timespans. This returns the same cubes in the current implementation, but raises an error in the new implementation.

Another issue that came up is related to the standard deviation and is described in #1024

Peter9192

Nice job @stefsmeets! I think these modified tests make the requirements for a new implementation much more explicit. As far as I can see, the new tests cover all of the old tests except for two of them. Could you have a look at how we could add those within the new test framework?

tests/unit/preprocessor/_multimodel/test_multimodel.py

Peter9192 · 2021-02-25T16:28:52Z

tests/unit/preprocessor/_multimodel/test_multimodel.py

+@pytest.mark.xfail(reason='Multimodel statistics returns the original cubes.')
+def test_edge_case_time_no_overlap_fail():
+    """Test case when time coords do not overlap using span='overlap'.
+
+    Expected behaviour: `multi_model_statistics` should fail if time
+    points are not overlapping.


I'm not sure if the desired behaviour here is to fail. I would say so if I was designing it from scratch, but maybe this is actually considered a 'feature' by some users/recipes?? Any thoughts on this @valeriupredoi @LisaBock

@schlunma what do you think?

My first intuition would also be to let it fail when there is no overlap, but I guess returning an empty cube (or no cube) could also be a reasonable option. I've never used that "feature", and I don't really know if anyone uses it. I would be fine with changing this behavior if it is not used anywhere.

Okay, let's leave it like this then 👍

remember the purpose of the test in general is to verify the integrity of the functional behavior, not to assume things on its own, so if the function fails if there is no overlap, the test should check for that 👍

Good point @valeriupredoi , the point of this PR is to establish the definition so that we have a better description of the functionality that we want. Currently there is no test that explicitly checks for this scenario, so the behaviour is not defined.

Therefore we want to clarify the desired behaviour with this PR. Since this return statement is inconsistent with the normal output (a list is returned rather than a dict with statistics), I marked this with XFAIL to indicate a known problem (which will be fixed with #968 😁).

If this is behaviour that we need with this function, that is also OK for me, then I will adjust the test to ensure that a list of cubes is given back if there is no overlap and update #968 accordingly.

sure, good call! I am perfectly fine with this as it is here, discussing further in the other PR is cool, the only bit that I think needs addressing is that array equality func below, other than that - good work here 🍺

schlunma · 2021-02-26T08:56:45Z

Hi guys, great job you're doing here!

I have a question regarding this PR: Should it already include tests for all functionalities that we want the future multimodel_statistics preprocessor to have (and mark those that fail right now with an xfail)? If no, you can stop reading 😄

If yes, do we want want to include tests that ensure that the preprocessor works with cube that have arbitrary coordinates? I think that's a really desired features (#1018 #891 #890). Right now, I think all the tests include the time coordinate somehow (please correct me if I'm wrong). I can add some tests to this branch directly if you agree!

Peter9192 · 2021-02-26T09:17:18Z

Hi @schlunma yes I think this test-driven development makes a lot of sense! It helps to map all the requirements for a new implementation.

schlunma · 2021-02-26T09:29:11Z

Cool, sounds great! I will add some tests later today.

stefsmeets · 2021-02-26T09:34:07Z

Hi @schlunma , we already have tests for #890 and #891

ESMValCore/tests/sample_data/multimodel_statistics/test_multimodel.py

Line 227 in fee8d93

def test_multimodel_no_horizontal_dimension(timeseries_cubes_month):

ESMValCore/tests/sample_data/multimodel_statistics/test_multimodel.py

Line 251 in fee8d93

def test_multimodel_no_time_dimension(timeseries_cubes_month):

Peter9192 · 2021-02-26T09:36:34Z

What's maybe even more important than tests for new desired features, is tests for 'hidden' requirements. We recently saw an example of this in #975. Are there any other 'features' or 'requirements' you can think are currently not covered by the tests?

schlunma · 2021-02-26T09:44:53Z

Hi @schlunma , we already have tests for #890 and #891

Ahh, nice, so that should be covered then.

What's maybe even more important than tests for new desired features, is tests for 'hidden' requirements. We recently saw an example of this in #975. Are there any other 'features' or 'requirements' you can think are currently not covered by the tests?

Not really. One thing that recently came up is #1009 (handling of altitude coordinate), but once the preprocessor can handle arbitrary coordinates this should not be an issue anymore.

schlunma · 2021-02-26T09:48:50Z

One suggestion: Since we want to support cubes with no time coordinate, I think that the span argument should be optional in the future. I really doesn't make sense to specify this when there is no time dimensions.

Peter9192 · 2021-03-01T08:22:18Z

One suggestion: Since we want to support cubes with no time coordinate, I think that the span argument should be optional in the future. I really doesn't make sense to specify this when there is no time dimensions.

I like the idea, but I'm not sure we'll address this in #968 already. I think it sounds more like a follow up. But we can certainly open an issue about it and maybe already start thinking about a test that belongs to it.

stefsmeets · 2021-03-01T10:06:07Z

Hi @ESMValGroup/esmvaltool-coreteam , I think this PR is ready to be merged!

valeriupredoi

@stefsmeets good job, tests are looking much more comprehensive now! Just a wee change related to the array equality checker func and a comment related to the overlap check from me 🍺

valeriupredoi · 2021-03-01T10:20:22Z

tests/unit/preprocessor/_multimodel/test_multimodel.py

+@pytest.mark.xfail(reason='Multimodel statistics returns the original cubes.')
+def test_edge_case_time_no_overlap_fail():
+    """Test case when time coords do not overlap using span='overlap'.
+
+    Expected behaviour: `multi_model_statistics` should fail if time
+    points are not overlapping.


remember the purpose of the test in general is to verify the integrity of the functional behavior, not to assume things on its own, so if the function fails if there is no overlap, the test should check for that 👍

valeriupredoi · 2021-03-01T10:24:17Z

tests/unit/preprocessor/_multimodel/test_multimodel.py

+    if np.ma.isMaskedArray(this) or np.ma.isMaskedArray(other):
+        np.testing.assert_array_equal(this.mask, other.mask)
+
+    np.testing.assert_allclose(this, other)


any reason why you add the same function as the one from the tests' init file here? well, you are adding the allclose check to it, but why do you need it? please use the imported function and if there is a need for an almost equal check, add it in the test that needs it

Hi @valeriupredoi , I'm not entirely sure how to import this file without messing with sys.path. The default way for defining imports in pytest is in conftest.py, which we do not use atm.

no need to import per se it's already imported at module run level, just use it from there without duplicating it here

Like you said, the functions are different (array_allclose vs array_equal), so the other one cannot be used directly. Instead, I changed the function name to better reflect its function. Does that work for you?

OK we should move this one upstream in the init at some point and just use it from there

valeriupredoi · 2021-03-01T10:27:19Z

oh and another comment: could you please tell me what's the coverage percentage now compared to the previous module?

valeriupredoi · 2021-03-01T10:31:44Z

One suggestion: Since we want to support cubes with no time coordinate, I think that the span argument should be optional in the future. I really doesn't make sense to specify this when there is no time dimensions.

we should account for a nospan or notime case, but making the argument fully optional may lead to user errors when they forget to set it and we'd have to make the function use a default span - there are major differences between, overlap and full, and even if you throw a warning that complains the span parameter was not set, defaulting to blah, users don't read warnings

Peter9192 · 2021-03-01T10:44:12Z

One suggestion: Since we want to support cubes with no time coordinate, I think that the span argument should be optional in the future. I really doesn't make sense to specify this when there is no time dimensions.

we should account for a nospan or notime case, but making the argument fully optional may lead to user errors when they forget to set it and we'd have to make the function use a default span - there are major differences between, overlap and full, and even if you throw a warning that complains the span parameter was not set, defaulting to blah, users don't read warnings

One way to do it is to add an additional (not the default) option that says span=None if you don't want it. But again, let's discuss that in another issue.

valeriupredoi · 2021-03-01T10:51:02Z

One suggestion: Since we want to support cubes with no time coordinate, I think that the span argument should be optional in the future. I really doesn't make sense to specify this when there is no time dimensions.

we should account for a nospan or notime case, but making the argument fully optional may lead to user errors when they forget to set it and we'd have to make the function use a default span - there are major differences between, overlap and full, and even if you throw a warning that complains the span parameter was not set, defaulting to blah, users don't read warnings

One way to do it is to add an additional (not the default) option that says span=None if you don't want it. But again, let's discuss that in another issue.

yip, agreed on both counts!

stefsmeets · 2021-03-01T11:32:24Z

oh and another comment: could you please tell me what's the coverage percentage now compared to the previous module?

For _multimodel.py,
this PR: statements: 196, missing: 13, coverage: 93%,
master: statements: 196, missing: 17, coverage: 91%

Note that the coverage says very little about the quality of the tests.

valeriupredoi · 2021-03-01T11:39:22Z

shweet! yes, it's a number that measures brute coverage, just wanted to check there are no omissions in statement checks that were done before, this PR introduces a better quality test module 👍

valeriupredoi · 2021-03-01T12:51:53Z

cheers for the good work @stefsmeets 🍺

Update multimodel preprocessor tests

7fd3096

stefsmeets added preprocessor Related to the preprocessor testing labels Feb 25, 2021

Mark known failures with xfail

682cdee

Peter9192 requested changes Feb 25, 2021

View reviewed changes

Update reason with github issue

56c6ebf

Address review comments

ad3e3c0

stefsmeets requested a review from Peter9192 February 26, 2021 11:16

Peter9192 approved these changes Mar 1, 2021

View reviewed changes

valeriupredoi requested changes Mar 1, 2021

View reviewed changes

Rename array assert function

3a3efb2

valeriupredoi approved these changes Mar 1, 2021

View reviewed changes

valeriupredoi merged commit 2966f54 into master Mar 1, 2021

valeriupredoi deleted the mmstats_update_tests branch March 1, 2021 12:51

tomaslovato mentioned this pull request Mar 8, 2021

Preprocessor chain 'climate_statistics' and 'multimodel_statistics' fails as time dimension is not present #1018

Closed

Peter9192 mentioned this pull request Jun 30, 2021

New multimodel module can be slow and buggy for certain recipes #1201

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update tests for multimodel statistics preprocessor #1023

Update tests for multimodel statistics preprocessor #1023

stefsmeets commented Feb 25, 2021 •

edited

Loading

stefsmeets commented Feb 25, 2021 •

edited

Loading

Peter9192 left a comment

Peter9192 Feb 25, 2021

Peter9192 Feb 26, 2021

schlunma Feb 26, 2021

Peter9192 Mar 1, 2021

valeriupredoi Mar 1, 2021

stefsmeets Mar 1, 2021

valeriupredoi Mar 1, 2021 •

edited

Loading

schlunma commented Feb 26, 2021

Peter9192 commented Feb 26, 2021

schlunma commented Feb 26, 2021

stefsmeets commented Feb 26, 2021 •

edited

Loading

Peter9192 commented Feb 26, 2021

schlunma commented Feb 26, 2021

schlunma commented Feb 26, 2021

Peter9192 commented Mar 1, 2021

stefsmeets commented Mar 1, 2021

valeriupredoi left a comment

valeriupredoi Mar 1, 2021

valeriupredoi Mar 1, 2021

stefsmeets Mar 1, 2021 •

edited

Loading

valeriupredoi Mar 1, 2021

stefsmeets Mar 1, 2021

valeriupredoi Mar 1, 2021

valeriupredoi commented Mar 1, 2021

valeriupredoi commented Mar 1, 2021

Peter9192 commented Mar 1, 2021

valeriupredoi commented Mar 1, 2021

stefsmeets commented Mar 1, 2021

valeriupredoi commented Mar 1, 2021

valeriupredoi commented Mar 1, 2021

Update tests for multimodel statistics preprocessor #1023

Update tests for multimodel statistics preprocessor #1023

Conversation

stefsmeets commented Feb 25, 2021 • edited Loading

Description

Before you get started

Checklist

stefsmeets commented Feb 25, 2021 • edited Loading

Peter9192 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

valeriupredoi Mar 1, 2021 • edited Loading

Choose a reason for hiding this comment

schlunma commented Feb 26, 2021

Peter9192 commented Feb 26, 2021

schlunma commented Feb 26, 2021

stefsmeets commented Feb 26, 2021 • edited Loading

Peter9192 commented Feb 26, 2021

schlunma commented Feb 26, 2021

schlunma commented Feb 26, 2021

Peter9192 commented Mar 1, 2021

stefsmeets commented Mar 1, 2021

valeriupredoi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stefsmeets Mar 1, 2021 • edited Loading

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

valeriupredoi commented Mar 1, 2021

valeriupredoi commented Mar 1, 2021

Peter9192 commented Mar 1, 2021

valeriupredoi commented Mar 1, 2021

stefsmeets commented Mar 1, 2021

valeriupredoi commented Mar 1, 2021

valeriupredoi commented Mar 1, 2021

stefsmeets commented Feb 25, 2021 •

edited

Loading

stefsmeets commented Feb 25, 2021 •

edited

Loading

valeriupredoi Mar 1, 2021 •

edited

Loading

stefsmeets commented Feb 26, 2021 •

edited

Loading

stefsmeets Mar 1, 2021 •

edited

Loading