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

resample idx #354

Merged
merged 11 commits into from
Apr 21, 2020
Merged

resample idx #354

merged 11 commits into from
Apr 21, 2020

Conversation

aaronspring
Copy link
Collaborator

@aaronspring aaronspring commented Apr 19, 2020

Description

This implements the index based resampling developed by @ahuang11 and @bradyrx for the stats threshold functions. Next PR would refactor bootstrap: first resample (based on resample_idx) input, then apply metric, see https://github.com/bradyrx/climpred/pull/355

also added new asv benchmarks for 1D data

breaking change: keyword change: bootstrap -> iterations

works on #145

Type of change

Please delete options that are not relevant.

  • Performance (if you touched existing code run asv to detect performance changes)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. This could point to a cell in the updated notebooks. Or a snippet of code with accompanying figures here.

Checklist (while developing)

  • I have added docstrings to all new functions.
  • I have commented my code, particularly in hard-to-understand areas
  • Tests added for pytest, if necessary.

Pre-Merge Checklist (final steps)

  • I have rebased onto master or develop (wherever I am merging) and dealt with any conflicts.
  • I have squashed commits to a reasonable amount, and force-pushed the squashed commits.

References

your gist

@coveralls
Copy link

coveralls commented Apr 19, 2020

Coverage Status

Coverage increased (+0.04%) to 90.849% when pulling 126d967 on AS_resample_fast into f956be3 on master.

climpred/bootstrap.py Outdated Show resolved Hide resolved
climpred/bootstrap.py Outdated Show resolved Hide resolved
climpred/bootstrap.py Outdated Show resolved Hide resolved
climpred/bootstrap.py Outdated Show resolved Hide resolved
climpred/bootstrap.py Show resolved Hide resolved
climpred/bootstrap.py Outdated Show resolved Hide resolved
climpred/tests/test_bootstrap.py Show resolved Hide resolved
climpred/tests/test_stats.py Outdated Show resolved Hide resolved
@bradyrx
Copy link
Collaborator

bradyrx commented Apr 19, 2020

This looks great @aaronspring. Just some nitpicky naming stuff to make things more clear for future development and a question about this zero issue in the resampling dimension.

Tested some on Cheyenne and it's all running fast. This will set things up nicely to implement this in bootstrapping for skill.

@aaronspring
Copy link
Collaborator Author

I change now got bigger as I replaced the bootstrap keyword with iterations. there are some minor bugs in the documentation regarding compute_persistence and compute_uninit because they are not in compute but now in reference.

@aaronspring
Copy link
Collaborator Author

aaronspring commented Apr 21, 2020

bigger performance increase ever. 0.02 is a 50x increase. I would have expected to see this in the next PR, but we also see this here already.

       before           after         ratio
     [8be0a893]       [4a2cc68a]
     <AS_adapt_xr0151~2>       <AS_resample_fast~1>
-       23.1±0.1s         455±20ms     0.02  benchmarks_perfect_model.ComputeSmall.time_bootstrap_perfect_model('pearson_r', 'm2c')
-         24.6±1s         484±20ms     0.02  benchmarks_perfect_model.ComputeSmall.time_bootstrap_perfect_model('crpss', 'm2m')
-       21.0±0.6s          409±5ms     0.02  benchmarks_perfect_model.ComputeSmall.time_bootstrap_perfect_model('rmse', 'm2c')
-       32.8±0.2s         636±20ms     0.02  benchmarks_perfect_model.ComputeSmall.time_bootstrap_perfect_model('rmse', 'm2m')
-       35.2±0.4s         680±20ms     0.02  benchmarks_perfect_model.ComputeSmall.time_bootstrap_perfect_model('pearson_r', 'm2m')

SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY.
PERFORMANCE INCREASED.

EDIT. there must be something wrong with this. the next PR will have some serious asv

@aaronspring
Copy link
Collaborator Author

and I didnt touch the documentation here where it fails. the new file reference.py is not implemented for api and also on the .rst files when referencing compute_persistence @bradyrx

@bradyrx bradyrx merged commit 2bc9cd5 into master Apr 21, 2020
@aaronspring aaronspring deleted the AS_resample_fast branch July 6, 2020 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants