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

Speed up multimodel statistics and fix bug in peak computation #1301

Merged
merged 1 commit into from
Sep 6, 2021

Conversation

bouweandela
Copy link
Member

@bouweandela bouweandela commented Sep 2, 2021

Description

This reduces the runtime of the multi_model_statistics preprocessor function in some cases by reducing the number of steps that need to be taken to iterate over the time dimension.

I think there was a bug in the computation of the peak statistic, which resulted in a cube with the wrong dimensions. @Peter9192 Could you check if the new behavior is correct, since you added the test for that in #1150?


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:

@bouweandela bouweandela added the bug Something isn't working label Sep 2, 2021
@codecov
Copy link

codecov bot commented Sep 2, 2021

Codecov Report

Merging #1301 (e1d0559) into main (c252b88) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1301      +/-   ##
==========================================
+ Coverage   86.46%   86.47%   +0.01%     
==========================================
  Files         188      188              
  Lines        9196     9208      +12     
==========================================
+ Hits         7951     7963      +12     
  Misses       1245     1245              
Impacted Files Coverage Δ
esmvalcore/preprocessor/_multimodel.py 94.96% <100.00%> (+0.41%) ⬆️

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 c252b88...e1d0559. Read the comment docs.

@Peter9192
Copy link
Contributor

Seems that peak tests was added here: db3aa93. I'm not sure why it behaved differently than others, @stefsmeets do you recall?

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.

took me a few minutes to think of all the corner cases and do arithmetic in me head (not easy, the latter 😆 )

@stefsmeets
Copy link
Contributor

Seems that peak tests was added here: db3aa93. I'm not sure why it behaved differently than others, @stefsmeets do you recall?

I can't remember this change exactly. Defining the outcome of the 'peak' function was not in the scope of the PR, so we probably left the result as it was.

@bouweandela bouweandela merged commit 478ba30 into main Sep 6, 2021
@bouweandela bouweandela deleted the multi-model-statistics-performance branch September 6, 2021 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants