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

Make sure climate statistics always returns original dtype #1237

Merged
merged 4 commits into from
Jul 20, 2021

Conversation

zklaus
Copy link

@zklaus zklaus commented Jul 19, 2021

Description

This adds code to the climate_statistics preprocessor to detect whether it has changed the dtype of the cube. If this is the case, it changes the dtype back to the original dtype.

Closes #1235

Link to documentation:


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:

@zklaus zklaus added bug Something isn't working preprocessor Related to the preprocessor labels Jul 19, 2021
@codecov
Copy link

codecov bot commented Jul 20, 2021

Codecov Report

Merging #1237 (9d341bc) into main (2a0bb11) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1237   +/-   ##
=======================================
  Coverage   85.54%   85.54%           
=======================================
  Files         188      188           
  Lines        9150     9154    +4     
=======================================
+ Hits         7827     7831    +4     
  Misses       1323     1323           
Impacted Files Coverage Δ
esmvalcore/preprocessor/_time.py 94.21% <100.00%> (+0.07%) ⬆️

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 2a0bb11...9d341bc. Read the comment docs.

else:
clim_cube = iris.cube.CubeList(
clim_cube.slices_over(clim_coord.name())).merge_cube()
cube.remove_coord(clim_coord)
Copy link
Author

Choose a reason for hiding this comment

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

This line looks a bit fishy to me, but it was there before, so there is that. It looks like this should be clim_cube.remove_coord(clim_coord). @valeriupredoi, this seems to come from you. Do you have an opinion?

@jvegreg
Copy link
Contributor

jvegreg commented Jul 20, 2021

I am changing the tests in the time module to always check for the types

@jvegreg
Copy link
Contributor

jvegreg commented Jul 20, 2021

I have detected the same problem when computing standardized anomalies, as they get converted to float64 from a float32 input. I am trying to solve it and then I will push the changes

@zklaus
Copy link
Author

zklaus commented Jul 20, 2021

I have detected the same problem when computing standardized anomalies, as they get converted to float64 from a float32 input. I am trying to solve it and then I will push the changes

Be advised that I came to this solution via the anomalies. Turns out the problem with anomalies was that the climate_statistics, which are used in the calculation of the anomalies did this conversion, so maybe once this PR is in, the anomalies problem vanishes.

@jvegreg
Copy link
Contributor

jvegreg commented Jul 20, 2021

Be advised that I came to this solution via the anomalies. Turns out the problem with anomalies was that the climate_statistics, which are used in the calculation of the anomalies did this conversion, so maybe once this PR is in, the anomalies problem vanishes.

I am working directly on this PR, so that's not the issue

Copy link
Contributor

@jvegreg jvegreg left a comment

Choose a reason for hiding this comment

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

Nice catch! I currently do not have access to data (BSC annual maintenace) to properly test with a recipe, though.

@zklaus zklaus merged commit 96ee680 into main Jul 20, 2021
@zklaus zklaus deleted the fix-climate-statistics-dtype branch July 20, 2021 11:51
@jvegreg jvegreg mentioned this pull request Jul 20, 2021
10 tasks
zklaus pushed a commit that referenced this pull request Jul 21, 2021
* Make sure climate statistics always returns original dtype

* Homogenize different period branches

* Change climate_statistics tests to consistently use float32

* Change logging style to old logging style
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working preprocessor Related to the preprocessor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The climate_statistics preprocessor sometimes changes the dtype
2 participants