Conversation
|
The benchmarks have registered a significant increase in performance, which is reassuring... Although they don't specifically target the use case of #4538. That said, this change seems like a general good win IMHO |
|
I'll add an appropriate |
wjbenfold
left a comment
wjbenfold
left a comment
There was a problem hiding this comment.
Changes to _merge and cube look good to me
|
FWIW, I can't see an explicit test of the |
|
I don't believe we need an explicit test for this, it's more than exercised with existing tests. But that's just my opinion 😉 |
|
BTW I'm pretty much AWTK today, so don't let a lack of If you've got capacity, and think it's worthwhile, perhaps add one as a follow on? |
| for coord in sorted(coords, key=key_func): | ||
| if not cube.coord_dims(coord) and coord.shape == (1,): | ||
| dims = tuple(cube.coord_dims(coord)) | ||
| if not dims and coord.shape == (1,): |
There was a problem hiding this comment.
FWIW I was looking at this only yesterday, in quite another context
And, I think that the shape didn't really need testing :
if it has no cube-dims, it must be scalar, no?
* main: (64 commits) [pre-commit.ci] pre-commit autoupdate (SciTools#4587) fix test (SciTools#4585) Loading Benchmarks (SciTools#4477) Fix refresh lockfile worrkflow pull request title (SciTools#4579) gha: lockfiles labels and auto-pr details (SciTools#4578) Bump actions/script from 4 to 5.1.0 (SciTools#4576) Bump actions/stale from 4.0.0 to 4.1.0 (SciTools#4575) Bump peter-evans/create-pull-request from 3.8.2 to 3.12.1 (SciTools#4577) adopt dependabot GHA (SciTools#4568) New tool-agnostic ASV environment management (SciTools#4571) Updated environment lockfiles (SciTools#4567) Update version to 3.3.dev0 (SciTools#4565) Reset whats new (SciTools#4563) docs: move whatsnew (SciTools#4540) Deprecate regrids (SciTools#4548) Update latest.rst (SciTools#4556) Updated environment lockfiles (SciTools#4555) Added whatsnew for SciTools#4551. (SciTools#4553) Update SciTools#4505 doctests for SciTools#4528. (SciTools#4554) minor optimisations (SciTools#4549) ...
3 participants
🚀 Pull Request
Description
This PR introduces an optimisation to
iris.cube.Cube.coord_dims, and also a lesser optimisation to merge.This change contributes, in part, to addressing the use case raised by issue #4538. As a result, I've seen a rough 25% speed-up of the worst case scenario given in #4538 (on my desktop) i.e., loading a 24,000 field PP file (time series) has gone from ~80secs to ~60secs.
Essentially, a cheap win saving can be achieved by not blindly calling
iris.cube.Cube.coordwithiniris.cube.Cube.coord_dims, particularly when the actual cube coordinate that exists within the cube is provided to thecoord_dimsmethod.The philosophy of
coord_dimsis to avoid coordinate equivalence, as it is a relatively expensive operation, but rather use object identity instead, if possible.Consult Iris pull request check list