Intersection bounds fix (replacement PR) #4059

rcomer · 2021-03-10T15:07:44Z

🚀 Pull Request

Description

Replacement for #4046, targetting v3.0.x

Fixes #3391 I think. This addresses the very specific case where you request the entire 360 degrees, and one of the bounds is at the maximum/minimum threshold. Taking @valeriupredoi's example from #3391, you would still get that negative longitude point if you did something like cube.intersection(longitude=(0, 50)). I'm not sure that that makes sense but, looking at the tests, it appears to have been intentional.

I haven't added any extra comment to explain my logic, but I think it's covered by the existing:

iris/lib/iris/cube.py

Lines 3178 to 3180 in a9ae72f

    
           # being split. For bounds which may extend exactly the length of 
        
           # the modulus, we simply preserve the point to bound difference, 
        
           # and call the new bounds = the new points + the difference.

Consult Iris pull request check list

jamesp

Looks great, the new test passes with the new lib/iris/cube.py, and fail without it 👍

One question inline, due to my unfamiliarity with the whole codebase. If it's all good, LGTM

jamesp · 2021-05-06T08:36:32Z

lib/iris/cube.py

+                    cells_delta = np.diff(coord.bounds[indices])
+
+                    # Watch out for ascending/descending bounds.
+                    if cells_delta[0, 0] > 0:


Previously, minimum would have been set by either branch of this if statement on every call. Now that it is contained within if maximum % modulus not in cells it may not be. Should there be an else to the if on L3135?

It's only been two months but I had completely forgotten the logic here! I think this is fine though: we don't explicitly set minimum here, so it is just left to whatever minimum was originally passed into the method. Note that because of the previous logic

maximum - modulus <= minimum (line 3081)
cells >= minimum (line 3098)

So the only way that maximum - modulus in cells will be True is if maximum - modulus == minimum and minimum is in cells. I.e. the very specific case described in #3391. In that case, it is expected that the bounds on the output coordinate will precisely span the requested (minimum, maximum) range.

Definitely needs a comment though!

jamesp · 2021-05-06T08:37:18Z

lib/iris/cube.py

-                else:
-                    cells[:, 1] = cells[:, 0] + cells_delta[:, 0]
-                    minimum = np.min(cells[:, 1])
+                if maximum % modulus not in cells:


Perhaps a comment here, even referencing the exact problem that this code is aiming to solve.

rcomer · 2021-05-06T09:48:11Z

Now my tests are failing because this is targetting the v3.0.x branch, so doesn't have any of the CI goodness that's just been added to master. 😔

jamesp · 2021-05-06T09:53:55Z

Oh no! well you've only added comments so functionally the same as before.

We should probably decide if we want to cherry pick some of the CI changes into the v3.0.x branch

cc. @bjlittle @trexfeathers

valeriupredoi · 2021-05-11T11:49:32Z

Now my tests are failing because this is targetting the v3.0.x branch, so doesn't have any of the CI goodness that's just been added to master. pensive

merge master in here?

rcomer · 2021-05-13T09:31:44Z

Rebased. Main tests pass locally under py3.6 and py3.7 py3.8 tests look to be running OK in Cirrus.

py3.6

Ran 5036 tests in 307.194s

OK (SKIP=22)

py3.7

Ran 5017 tests in 513.501s

OK (SKIP=22)

py3.7 really seems to be misplacing tests....

rcomer · 2021-05-13T09:50:34Z

Doc link check and gallery test are same as for #4130. The gallery test still passes locally for me under py3.8.

rcomer · 2021-05-13T14:35:36Z

@jamesp I think this is now the best I can do with this one.

jamesp · 2021-05-13T14:45:27Z

Happy with this, LGTM

@trexfeathers

* Add release highlights and pin rc version (#3898) * Add release highlights and pin rc version * review actions * reorder release highlights (#3899) Tweak release highlights * Add whatsnew announcement (#3900) * Fix spelling (#3903) * Fix unit label handling (#3902) * Add failing test of plotting * Implement fix to pass test * Update idiff to ignore irrelevant hyphens in path * Update imagerepo (following docs) * Update after review by @trexfeathers * Add whatsnew entries * Move whatsnew entries into correct file * Release Docs Improvements (#3895) * Minor phrasing change in 'Release candidate'. * Before release deprecations. * Whatsnew highlights section. * Relax setup.py setup requirements (#3909) * Updated CF saver version in User Guide and docstring (#3925) * Updated CF saver version in User Guide and docstring * Remove references to CF version of the loader in docstrings * Added whatsnew * Pin cftime<1.3.0 * Migrate to cirrus-ci (#3928) * migrate from travis-ci to cirrus-ci * added whatsnew entries * ignore url for doc link check (#3929) * whatsnew for coord default units (#3924) * Cube._summary_coord_extra: efficiency and bugfix (#3922) * Add Documentation Title Case Capitalization (#3940) * Use Title Case Capitalisation for Documentation * add whatsnew enter * CI requirements drop pip packages (#3939) * requirements pip to conda * use pip install over develop * default PY_VER to python versions * update links (#3942) * update links * added s to http * Add support for 1-d weights in collapse. (#3943) * Remove warning for convert_units on lazy data (#3951) * drop stickler references in docs (#3953) * drop stickler references in docs * remove sticker from common links * update docs for travis-ci to cirrus-ci (#3954) * update docs for travis-ci to cirrus-ci * add 'travis-ci' reference locally to whatsnew * update whatsnew comment * docs for nox (#3955) * docs for nox * add titles, notices and additional detail * review actions * Resolve test coverage (#3947) * test coverage for __init__ and __call__ * test coverage for metadata resolve and coverage * partial test coverage for metadata mapping * python 3.6 workaround for deepcopy of mock.sentinel * test coverage for Resolve._free_mapping * test coverage for Resolve convenience methods * add test stub for Resolve._metadata_mapping * fix Test__tgt_cube_position * test coverage for shape * test coverage for _as_compatible_cubes * test coverage for Resolve._metadata_mapping * test coverage for Resolve._prepare_common_dim_payload * test coverage for Resolve._prepare_common_aux_payload * test coverage for Resolve._prepare_points_and_bounds * test coverage for Resolve._create_prepared_item * test coverage for Resolve._prepare_local_payload_dim * test coverage for Resolve._prepare_local_payload_aux * test coverage for Resolve._prepare_local_payload_scalar + docs URL skip * test coverage for Resolve._prepare_local_payload * test coverage for Resolve._metadata_prepare * added docs URL linkcheck skip * test coverage for Resolve._prepare_factory_payload * test coverage for Resolve._get_prepared_item * review actions * test coverage for Resolve.cube * pin v3.0.0 version and whatnew date (#3956) * update github ci checks image (#3957) * Promote unknown units to dimensionless in aux factories (#3965) * promote unknown to dimensionless units in aux factories * patch aux factories to promote unknown to dimensionless units for formula terms * add whatnew PR for entry * Release branch prepare for v3.0.2 (#4044) * update intersphinx mapping and matplotlib urls (#4003) * update intersphinx mapping and matplotlib urls * use matplotlib intersphinx where possible * review actions * review actions * cirrus-ci compute credits (#4007) * cirrus-ci conditional tasks (#4019) * cirrus-ci conditional tasks * use bc for bash arithmetic * revert back to sed * use expr * reword * minor documentation changes * review actions * prepare v3.0.2 release * Fix test_incompatible_dimensions test (#3977) * test_incompatible_dimensions used a ragged array for the test, which has been deprecated in numpy, and now fails if dtype is anything other than object. This test appears to be checking that the addition of a [2x4] masked array to a [2x3] masked cube should raise a ValueError. This commit fixes the creation of `data3` object to be a [2x4] non-ragged array. * Added entry to what's new * Added name to core developer list :) * Update latest.rst Fixed space in PR macro call * update whatsnew v3.0.2 Co-authored-by: James Penn <james@jamespenn.co.uk> * um_stash_source attribute improved handling (#4035) * Modified pyke rule * Tests added * Black and whatsnew * Include PR number * Remove latest.rst * Add what's new * Support for py38 and Cartopy 0.19 (#4130) * mpl 3.4.1 updates (#4087) * replace most recent hashes (#4112) * Corrected plot_anomaly_log_colouring for new Matplotlib linscale rules. (#4115) * Cartopy 0.19 updates (#4128) * Use assertArrayAllClose for sqrt test (#4118) * using AllClose for sqrt test * Omitting the checksum from test cml * use ArrayAllClose (rebase reset it?) * Iris py38 (#3976) * support for py38 * update CI and noxfile * enforce alphabetical xml element attribute order * full tests for py38 + fix docs-tests * add whatsnew entry * update doc-strings + review actions * Alternate xml handling routine (#29) * all xml tests pass for nox tests-3.8 * restored docstrings * move sort_xml_attrs * make sort_xml_attrs a classmethod * update sort_xml_attr doc-string Co-authored-by: Bill Little <bill.james.little@gmail.com> * add jamesp to whatsnew + minor tweak Co-authored-by: James Penn <james@jamespenn.co.uk> * reinstate black and nox links * Linkcheck update (#4104) * Updated links * Added login remark * Removed extra space * change to kick cirrus * kick cirrus * test verbose on cirrus * Removed test settings. Co-authored-by: Bill Little <bill.james.little@gmail.com> Co-authored-by: Martin Yeo <40734014+trexfeathers@users.noreply.github.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: James Penn <james.penn@metoffice.gov.uk> Co-authored-by: James Penn <james@jamespenn.co.uk> Co-authored-by: tkknight <2108488+tkknight@users.noreply.github.com> * Intersection bounds fix (replacement PR) (#4059) * fix intersection out of bounds point * fix intersection out of bounds point take 2 * add referencing comment * bootstrap ci for 3.0.x (#4154) * add cirrus-ci docker support * wip * wip * wip * wip * wip * add pip * revert docker * fix add_weekday (#32) * fix add_weekday * fix black link * fix link Co-authored-by: Ruth Comer <10599679+rcomer@users.noreply.github.com> * Tweak to speed up dask wrapping of netcdf variables (#4135) * Use 'meta' in da.from_array to stop it sampling netcdf variables, which is quite slow. * Fix PR number. * Fix test. * Review changes. * Update docs/iris/src/whatsnew/3.0.2.rst Co-authored-by: lbdreyer <lbdreyer@users.noreply.github.com> * Fb fix cube coord arithmetic (#4159) * fix coord with cube arithmetic * add coord arithmetic test coverage * add a whatsnew entry * Pp daskfix (#4141) * Remove workaround when dask-wrapping PP data, obsoleted by #4135. * Remove old slice testing * Add whats new * move whitespace? * missing line Co-authored-by: lbdreyer <laura.dreyer@metoffice.gov.uk> * add release date to v3.0.2 whatsnew (#4160) * update readme logo img src and href (#4006) (#4216) * In cube.intersection, find split cells using a tolerant equality check (#4220) * In cube.intersection, find split cells using a tolerant equality check * remove unused var * Add bounds check to tests; add what's new * Fix whats new; update version number * Update 3.0.3.rst Update release date in `3.0.3.rst` * Wide cubestr fix 3v0vx v2 (#4233) * Widen cube printout for long ancil or cell-measure names. * Adjust result for fixed cube-units printout. * Added whatsnew. * Include newest whatsnew in index. * Review: fix whatsnew structure. * Fix initial sections display. * Fix some typos in the cube maths docs (#4248) * (More of) Wide cubestr fix 3v0vx v2 (#4238) * Fix whatsnew for #4233. * Move 'empty slicings' whatsnew entry to 3.0.2 section. * unpin cftime (#4222) * Test wrangling vs v3.0.x (#4249) * test changes * add awol init * rename system test cases * fix PartialDateTime tests * fix typo * add whatsnew * Fix mergeback PR #4035 * Fix mergeback PR #4035 tests * Update mergeback nox conda-lock files Co-authored-by: tkknight <2108488+tkknight@users.noreply.github.com> Co-authored-by: Zeb Nicholls <zebedee.nicholls@climate-energy-college.org> Co-authored-by: Martin Yeo <40734014+trexfeathers@users.noreply.github.com> Co-authored-by: Jon Seddon <17068361+jonseddon@users.noreply.github.com> Co-authored-by: Ruth Comer <ruth.comer@metoffice.gov.uk> Co-authored-by: Patrick Peglar <patrick.peglar@metoffice.gov.uk> Co-authored-by: James Penn <james@jamespenn.co.uk> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: James Penn <james.penn@metoffice.gov.uk> Co-authored-by: Ruth Comer <10599679+rcomer@users.noreply.github.com> Co-authored-by: lbdreyer <lbdreyer@users.noreply.github.com> Co-authored-by: lbdreyer <laura.dreyer@metoffice.gov.uk>

rcomer force-pushed the intersection-bounds-fix-3.0.x branch from 99f32ab to 5646b16 Compare March 10, 2021 15:08

This was referenced Mar 10, 2021

fix intersection out of bounds point #4046

Closed

temporary fix to an issue with cube intersection when cube has bounds ESMValGroup/ESMValCore#800

Closed

rcomer assigned bjlittle Mar 10, 2021

rcomer added the Type: Bug label Mar 10, 2021

rcomer added this to In progress in Iris v3.0.x via automation Mar 10, 2021

rcomer linked an issue Mar 10, 2021 that may be closed by this pull request

cube.intersection out of bounds point #3391

Closed

jamesp reviewed May 6, 2021

View reviewed changes

bjlittle removed their assignment May 11, 2021

jamesp approved these changes May 11, 2021

View reviewed changes

Iris v3.0.x automation moved this from In progress to Reviewer approved May 11, 2021

rcomer added 3 commits May 13, 2021 10:13

fix intersection out of bounds point

a5d3793

fix intersection out of bounds point take 2

cf62f66

add referencing comment

71abfbd

rcomer force-pushed the intersection-bounds-fix-3.0.x branch from 5231c68 to 71abfbd Compare May 13, 2021 09:20

jamesp merged commit 05cd289 into SciTools:v3.0.x May 13, 2021

Iris v3.0.x automation moved this from Reviewer approved to Done May 13, 2021

rcomer deleted the intersection-bounds-fix-3.0.x branch May 13, 2021 14:47

This was referenced May 13, 2021

cube.intersection out of bounds point #3391

Closed

Fix broken links and gallery test #4132

Closed

valeriupredoi mentioned this pull request Jul 1, 2021

Cube coord bounds make cube intersection misbehave ESMValGroup/ESMValCore#799

Closed

This was referenced Jul 2, 2021

Intersection bounds when requested minimum is negative #4221

Closed

In cube.intersection, find split cells using a tolerant equality check #4220

Merged

vsherratt mentioned this pull request Oct 8, 2021

Assorted improvements to Cube.intersection #4363

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Intersection bounds fix (replacement PR) #4059

Intersection bounds fix (replacement PR) #4059

rcomer commented Mar 10, 2021

jamesp left a comment

jamesp May 6, 2021

rcomer May 6, 2021

jamesp May 6, 2021

rcomer commented May 6, 2021

jamesp commented May 6, 2021 •

edited

valeriupredoi commented May 11, 2021

rcomer commented May 13, 2021

rcomer commented May 13, 2021 •

edited

rcomer commented May 13, 2021

jamesp commented May 13, 2021

	# being split. For bounds which may extend exactly the length of
	# the modulus, we simply preserve the point to bound difference,
	# and call the new bounds = the new points + the difference.

Intersection bounds fix (replacement PR) #4059

Intersection bounds fix (replacement PR) #4059

Conversation

rcomer commented Mar 10, 2021

🚀 Pull Request

Description

jamesp left a comment

Choose a reason for hiding this comment

jamesp May 6, 2021

Choose a reason for hiding this comment

rcomer May 6, 2021

Choose a reason for hiding this comment

jamesp May 6, 2021

Choose a reason for hiding this comment

rcomer commented May 6, 2021

jamesp commented May 6, 2021 • edited

valeriupredoi commented May 11, 2021

rcomer commented May 13, 2021

rcomer commented May 13, 2021 • edited

rcomer commented May 13, 2021

jamesp commented May 13, 2021

jamesp commented May 6, 2021 •

edited

rcomer commented May 13, 2021 •

edited