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.

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

Test dtype in time preproc #1240

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Test dtype in time preproc #1240

wants to merge 7 commits into from

Conversation

jvegreg
Copy link
Contributor

@jvegreg jvegreg commented Jul 20, 2021

This is a follow up from #1237.

It add checks for dtype conservation on time preprocessor tests

Closes #

Before you get started

Checklist

It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the 🛠 Technical or 🧪 Scientific review.


To help with the number pull requests:

@jvegreg jvegreg requested a review from zklaus July 20, 2021 12:03
@jvegreg jvegreg added preprocessor Related to the preprocessor testing labels Jul 20, 2021
@codecov
Copy link

codecov bot commented Jul 20, 2021

Codecov Report

Merging #1240 (d755a9f) into main (b4f156c) will decrease coverage by 0.01%.
The diff coverage is 35.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1240      +/-   ##
==========================================
- Coverage   85.54%   85.53%   -0.02%     
==========================================
  Files         188      188              
  Lines        9154     9158       +4     
==========================================
+ Hits         7831     7833       +2     
- Misses       1323     1325       +2     
Impacted Files Coverage Δ
esmvalcore/preprocessor/_time.py 93.62% <35.71%> (-0.60%) ⬇️

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 b4f156c...d755a9f. Read the comment docs.

# standardize the results if requested
if standardize:
cube_stddev = climate_statistics(cube,
operator='std_dev',
Copy link
Contributor

Choose a reason for hiding this comment

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

eugh! we really need to deactivate in-line comments from codecov, it makes review really annoying 😖

Copy link
Contributor

Choose a reason for hiding this comment

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

son of a moose, if you just press "a" on the keyboard you can toggle the codecove comments 😁

Copy link
Contributor

@valeriupredoi valeriupredoi left a comment

Choose a reason for hiding this comment

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

nice ona Javi, one wee comment from me, it'd be good to print out an explanation if the dtype assertion fails

@@ -55,6 +51,12 @@ def _create_sample_cube(calendar='gregorian'):
return cube


def assert_equal_and_same_dtype(result, expected):
""""Check data is identical to the expected one."""
Copy link
Contributor

Choose a reason for hiding this comment

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

all the _time functions apart from climate_statistics should not alter the dtype but if they do, there is no explicit check for that inside the respective function, so the test will fail with a very obscure assertion error, it's a good idea to plop a print statement here a la "Checking for dtype consistency between original cube data and preprocessor result" and then the developer will know what hit them

tdim = cube.coord_dims('time')[0]
reps = cube.shape[tdim] / cube_stddev.shape[tdim]
if not reps % 1 == 0:
raise ValueError(
Copy link
Contributor

Choose a reason for hiding this comment

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

so I was trying to write a test for this statement here a la:

def test_standardized_anomalies_fail():
    """Test standardized ``anomalies`` failure due to shape mismatch."""
    cube = make_map_data(number_years=0.01)
    cube = cube[0:2]
    print(cube.coord("time"))
    with pytest.raises(ValueError):
        anomalies(cube, period="season", standardize=True)

which doesn't fail even if the cube's time axis is made up of two single points (two days) - surely this is something odd isn't it? Computing seasonal anomalies on two days should probably either fail or give the user a BIG warning data is totally unreliable?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preprocessor Related to the preprocessor testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants