Merged
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5926 +/- ##
==========================================
+ Coverage 89.78% 89.82% +0.03%
==========================================
Files 88 88
Lines 23060 23146 +86
Branches 5031 5043 +12
==========================================
+ Hits 20704 20790 +86
Misses 1624 1624
Partials 732 732 ☔ View full report in Codecov by Sentry. |
bouweandela
added
the
benchmark_this
Contributor
⏱️ Performance Benchmark Report: 11b4199Performance shiftsFull benchmark resultsGenerated by GHA run |
trexfeathers
reviewed
Jun 5, 2024
Member
Author
|
The benchmark report is a bit puzzling. I will look into it. Is there already a benchmark for |
Contributor
iris/benchmarks/benchmarks/merge_concat.py Lines 42 to 61 in d9c4c1d |
bouweandela
added
benchmark_this
Contributor
⏱️ Performance Benchmark Report: 060ea39Performance shiftsFull benchmark resultsGenerated by GHA run |
bouweandela
removed
the
benchmark_this
Member
Author
|
Thanks for the extensive review @trexfeathers! ✨ |
bouweandela
added
benchmark_this
Contributor
⏱️ Performance Benchmark Report: 758d9bePerformance shiftsFull benchmark resultsGenerated by GHA run |
trexfeathers
requested changes
Aug 29, 2024
Contributor
There was a problem hiding this comment.
@bouweandela I thoroughly approve of the changes you have made - nice job, and thanks very much for the comments/docstrings 💐
I have also decided - with some help - that the performance changes are a good trade-off 👍
- There is only 1 outstanding comment left from the first review: #5926 (comment)
I still haven't finished looking at the tests, but we're getting there...
trexfeathers
requested changes
Aug 30, 2024
Contributor
trexfeathers
left a comment
trexfeathers
left a comment
There was a problem hiding this comment.
OK I think I'm done with the review now. You have done an epic job - it was hard enough to review this, I can't imagine how difficult it was to write it!
Contributor
|
You might already be aware, but here is the final remaining action: #5926 (comment) |
Member
Author
Thanks, I was working on it 😉 Thanks a lot for the really extensive review! I think I have addressed all comments now. Should I run the benchmarks again? |
trexfeathers
added
benchmark_this
Contributor
⏱️ Performance Benchmark Report: 6ee351fPerformance shiftsFull benchmark resultsGenerated by GHA run |
trexfeathers
approved these changes
Sep 3, 2024
Draft
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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
Parallelize the comparison of the values of auxiliary coordinates, cell measures, ancillary variables, and derived coordinates during cube concatenation.
Closes #5750
Consult Iris pull request check list
Add any of the below labels to trigger actions on this PR: