Revert the default behaviour of coordinate realisation during pp load#7134
Revert the default behaviour of coordinate realisation during pp load#7134HGWright wants to merge 3 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7134 +/- ##
=======================================
Coverage 90.14% 90.14%
=======================================
Files 91 91
Lines 24967 24967
Branches 4684 4684
=======================================
Hits 22506 22506
Misses 1683 1683
Partials 778 778 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
pp-mo
left a comment
pp-mo
left a comment
There was a problem hiding this comment.
This all looks OK, except for 1 suggestion to improve a test clarity.
And.. I think you need to provide a whatsnew entry.
I think it needs to be fairly carefully worded, given the potential for confusion!
I also don't think we can afford to keep this quiet, as I see we did announce the original change
-- see in this whatsnew section.
| assert not data_cube.coord("surface_air_pressure").has_lazy_points() | ||
|
|
||
| # TODO: _CONCRETE_DERIVED_LOADING is a temporary fix, remove from test when a permanent fix exists | ||
| # TODO: _LAZY_DERIVED_LOADING is a temporary fix, remove from test when a permanent fix exists | ||
| load = mocker.Mock(return_value=iter([pressure_field, data_field])) | ||
| mocker.patch("iris.fileformats.pp.load", new=load) | ||
| with _CONCRETE_DERIVED_LOADING.context(): | ||
| _, realised_data_cube = iris.fileformats.pp.load_cubes("DUMMY") | ||
| assert not realised_data_cube.coord("surface_air_pressure").has_lazy_points() | ||
| with _LAZY_DERIVED_LOADING.context(load_lazy=True): | ||
| _, lazy_data_cube = iris.fileformats.pp.load_cubes("DUMMY") | ||
| assert lazy_data_cube.coord("surface_air_pressure").has_lazy_points() | ||
|
|
There was a problem hiding this comment.
I know it isn't your doing, but this test would be a lot clearer if it didn't test 2 things in 1.
Could we parametrise it?
Something like:
@pytest.mark.parametrize("islazy", [False, True], ids=["real", "lazy]")
...
with _LAZY_DERIVED_LOADING.context(load_lazy=islazy)
2 participants
🚀 Pull Request
Description
In #6767 we added a temporary fix, but we set the default to be lazy loading. We have since seen that this can cause some performance problems in some cases, so here we have reverted the default behaviour back to realising the data.
Part of the work needed to fix #7094. To complete that we would want a more permanent solution to this problem.
Consult Iris pull request check list
Add any of the below labels to trigger actions on this PR: