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

isin on asi8 instead of cftime #724

Merged
merged 4 commits into from
Feb 2, 2022

Conversation

aaronspring
Copy link
Collaborator

@aaronspring aaronspring commented Feb 1, 2022

Description

  • xr.DataArray.coords['init'].isin(xr.DataArray.coords['time']) is slow
  • dont use isin on xr.CFTimeIndex-backed xr.DataArray but after converting to index and asi8 (e.g. 'ms since 1970')

Closes #414

Type of change

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

How Has This Been Tested?

  • Tests added for pytest, if necessary.

Checklist (while developing)

  • I have added docstrings to all new functions.
  • CHANGELOG is updated with reference to this PR.

@aaronspring
Copy link
Collaborator Author

aaronspring commented Feb 1, 2022

Performance report:

on arm64-mac:

  • NMME ('lead': 12, 'init': 499)

    • commit 218cfaf speedup_dict
      benchmarks_PredictionEnsemble.NMME.time_verify
      metric
      -------- ---------
      rmse 406±5ms
      crps 442±1ms
    • commit 3e83811 main
      benchmarks_PredictionEnsemble.NMME.time_verify
      metric
      -------- ---------
      rmse 629±1ms
      crps 671±9ms
  • S2S ('init': 1060, 'lead': 46)

    • commit 218cfaf speedup_dict benchmarks_PredictionEnsemble.S2S.time_verify
      metric
      -------- ------------
      rmse 6.38±0s
      crps 6.38±0.04s

    • commit 3e83811 main benchmarks_PredictionEnsemble.S2S.time_verify
      metric
      -------- ------------
      rmse 42.3±0.08s
      crps 44.3±0.8s

    • before after ratio
      [3e83811] [218cfaf]

      42.3±0.08s 6.38±0s 0.15 benchmarks_PredictionEnsemble.S2S.time_verify('rmse')

SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY.
PERFORMANCE INCREASED.

on GHA:

  • NMME ('lead': 12, 'init': 499)
    • commit 218cfaf speedup_dict benchmarks_PredictionEnsemble.NMME.time_verify
      metric
      -------- -----------
      rmse 670±0.4ms
      crps 726±0.4ms
    • commit 3e83811 main benchmarks_PredictionEnsemble.NMME.time_verify
      metric
      -------- ---------
      rmse 1.04±0s
      crps 1.10±0s
  • S2S: configured not to run on GHA

@codecov
Copy link

codecov bot commented Feb 1, 2022

Codecov Report

Merging #724 (ec978d3) into main (dafd1f3) will decrease coverage by 0.45%.
The diff coverage is 85.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #724      +/-   ##
==========================================
- Coverage   93.80%   93.35%   -0.46%     
==========================================
  Files          59       59              
  Lines        6235     6263      +28     
==========================================
- Hits         5849     5847       -2     
- Misses        386      416      +30     
Impacted Files Coverage Δ
climpred/tests/test_bias_removal.py 97.66% <ø> (ø)
climpred/alignment.py 91.13% <73.68%> (-5.74%) ⬇️
climpred/checks.py 95.76% <100.00%> (ø)
climpred/classes.py 93.53% <100.00%> (+0.02%) ⬆️
climpred/metrics.py 91.73% <100.00%> (ø)
climpred/tests/test_alignment.py 100.00% <100.00%> (ø)
climpred/tests/test_relative_entropy.py 82.60% <100.00%> (-17.40%) ⬇️
climpred/graphics.py 83.90% <0.00%> (-12.07%) ⬇️
climpred/utils.py 90.39% <0.00%> (ø)

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 3e83811...ec978d3. Read the comment docs.

@aaronspring aaronspring marked this pull request as ready for review February 1, 2022 15:21
climpred/alignment.py Outdated Show resolved Hide resolved
@aaronspring aaronspring merged commit 76a1852 into pangeo-data:main Feb 2, 2022
@aaronspring aaronspring deleted the speedup_dict branch February 2, 2022 13:19
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.

Work out how to scale .verify() to lots of inits
1 participant