Simpler/faster data aggregation code in aggregated_by
#4970
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rcomer
commented
Sep 16, 2022
rcomer
commented
Sep 17, 2022
rcomer
force-pushed
the
simpler-aggregated_by
branch
from
ebea66b to
8667424
Compare
|
Ran the "cube" benchmarks locally and got |
rcomer
changed the title
aggregated_by codeaggregated_by
rcomer
force-pushed
the
simpler-aggregated_by
branch
from
e866cdf to
de848f8
Compare
rcomer
force-pushed
the
simpler-aggregated_by
branch
from
de848f8 to
52f6430
Compare
rcomer
added
the
benchmark_this
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #4970 +/- ##
==========================================
- Coverage 89.32% 89.31% -0.01%
==========================================
Files 89 89
Lines 22390 22373 -17
Branches 5374 5367 -7
==========================================
- Hits 20000 19983 -17
Misses 1640 1640
Partials 750 750
☔ View full report in Codecov by Sentry. |
rcomer
removed
the
benchmark_this
trexfeathers
reviewed
May 9, 2023
There was a problem hiding this comment.
Thanks @rcomer! Well done for making the code less scary. I've put a question in the comments, and will see if I can replicate your performance findings tomorrow.
I have been able to replicate this 👍 |
trexfeathers
approved these changes
May 11, 2023
There was a problem hiding this comment.
OK, I'm happy to merge as-is. Shame about the ❌ on the benchmark CI, but that's unavoidable until #5289 is finished.
tkknight
added a commit
to tkknight/iris
that referenced
this pull request
May 21, 2023
* upstream/main: Updated environment lockfiles (SciTools#5332) Support netcdf variable emulation (SciTools#5212) Support netCDF load+save on dataset-like objects as well as filepaths. (SciTools#5214) minor refinement to release do-nothing script (SciTools#5326) update bibtex citation for v3.6.0 release (SciTools#5324) Whats new updates for v3.6.0 (SciTools#5323) Added whatsnew notes on netcdf delayed saving and Distributed support. (SciTools#5322) Updated environment lockfiles (SciTools#5320) Bugfix 4566 (SciTools#4569) Simpler/faster data aggregation code in `aggregated_by` (SciTools#4970)
tkknight
added a commit
to tkknight/iris
that referenced
this pull request
Nov 9, 2023
* upstream/main: Updated environment lockfiles (SciTools#5334) Iris Docs: Dark theme ready (SciTools#5299) Updated environment lockfiles (SciTools#5332) Support netcdf variable emulation (SciTools#5212) Support netCDF load+save on dataset-like objects as well as filepaths. (SciTools#5214) minor refinement to release do-nothing script (SciTools#5326) update bibtex citation for v3.6.0 release (SciTools#5324) Whats new updates for v3.6.0 (SciTools#5323) Added whatsnew notes on netcdf delayed saving and Distributed support. (SciTools#5322) Updated environment lockfiles (SciTools#5320) Bugfix 4566 (SciTools#4569) Simpler/faster data aggregation code in `aggregated_by` (SciTools#4970) update release_do_nothing script (SciTools#5311) Restore latest Whats New files. Updated environment lockfiles (SciTools#5308) release_do_nothing script updates from v3.6.0rc0 (SciTools#5306) Whats new updates for v3.6.0rc0 (SciTools#5300) Bump scitools/workflows from 2023.04.3 to 2023.05.0 (SciTools#5303)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
🚀 Pull Request
Description
The code currently used for the lazy case in
aggregated_byjust needs a few tweaks to also use it for the real case. So we can have fewer lines, and the McCabe complexity of the method is reduced from 27 to 20.Edit: it turns out this also makes it faster (#4970 (comment)).
Consult Iris pull request check list