Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes to _discontiguity_in_bounds (attempt 2) #4975

Merged
merged 8 commits into from
Apr 12, 2023

Conversation

stephenworsley
Copy link
Contributor

🚀 Pull Request

Description

Addresses #3401 and #3480, replaces #3404 which I couldn't reopen since it's targeting a deleted branch.

This adresses three problems at once:

  • 2D discontiguity checking wasn't comparing enough bounds.
  • The logic for determining discontiguity was somewhat garbled, I couldn't figure out exactly what it was trying to achieve so I replaced it with np.isclose like the np.allclose in 1D checking.
  • It was unclear how the returned diffs ought to be represented as this used to be a boolean array for 2D coords and floats for 1D coords. Since this function is private, this is effectively a case of checking how these values are used. I believe that in each case they are used, they are being treated as an array of booleans so I have changed the behaviour of 1D coord checking and the description of the function to match this.

Consult Iris pull request check list

@rcomer
Copy link
Member

rcomer commented Sep 21, 2022

I haven't looked closely, but I think this would also address #4473?

@bjlittle bjlittle removed their assignment Feb 22, 2023
@ESadek-MO ESadek-MO requested a review from lbdreyer March 23, 2023 10:46
@trexfeathers trexfeathers requested review from pp-mo and removed request for lbdreyer April 3, 2023 09:23
Copy link
Member

@pp-mo pp-mo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After taking simply ages to understand what has changed here and why, I think I finally grasped it !
So, iris.util.find_discontiguity now finds an extra cell to be invalid in each testcase from iris.tests.unit.util.test_find_discontiguities, and the make_bounds_discontiguous_at_point test routine now encodes this too : although it makes the same coord adjustments as before, the resulting points which will be judged 'discontiguous' as a result have changed.

I have confirmed that the changes to the data mask in make_bounds_discontiguous_at_point correspond to those locations that now fail according to the utility routine find_discontiguities : Thefind_discontiguities code has not changed, but it relies on Coord._discontiguity_in_bounds, which of course has.

Most importantly, I have also confirmed that the new results from iris.util.find_discontiguities continue to perform as expected : The points which it marks are exactly those which are required to be masked to make iris.plot.pcolormesh succeed for the cubes with bounds discontiguitites generated by the iris.tests.unit.util.test_find_discontiguities tests.

So in effect, the results from iris.util.find_discontiguities have changed. But (a) the test code has been changed to match, and (b) the result still matches the requirement for "making mesh plots work".

I hope you agree with this analysis -- it has taken me a long time to convince myself that this is all correct + OK !

I think we can get away with this change, even though it breaks previous behaviour.
What is still missing is a whatsnew -- it should make clear that the results of "Coord.is_contiguous" and "util.find_discontiguity" have changed, but that we consider this to be a bugfix rather than a "breaking change".

I also have a slight qualm that the existing make_bounds_discontiguous_at_point routine always modifies two corners at once.
I think that may have made sense before, when we only compared bounds 0+1' in X and 3+0' in Y, but now it would make more sense to have independent testcases that check the "new comparisons" -- i.e. 2+3' in X and 2+1' in Y.
So, I suggest you should enhance iris.tests.stock._stock_2d_latlons.make_bounds_discontiguous_at_point to do these, and add suitable testcases to the existing iris.tests.unit.util.test_find_discontiguities.Test.test_find_discontiguities and iris.tests.unit.util.test_find_discontiguities.Test.test_find_discontiguities_in_y.

lib/iris/tests/stock/_stock_2d_latlons.py Outdated Show resolved Hide resolved
lib/iris/coords.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Apr 6, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01 ⚠️

Comparison is base (333a3a9) 89.27% compared to head (19fa7dc) 89.26%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4975      +/-   ##
==========================================
- Coverage   89.27%   89.26%   -0.01%     
==========================================
  Files          88       88              
  Lines       22261    22264       +3     
  Branches     4873     4874       +1     
==========================================
+ Hits        19874    19875       +1     
- Misses       1641     1642       +1     
- Partials      746      747       +1     
Impacted Files Coverage Δ
lib/iris/util.py 89.79% <ø> (-0.35%) ⬇️
lib/iris/coords.py 93.18% <100.00%> (-0.02%) ⬇️

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@pp-mo pp-mo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think there are still a few more minor changes to make.
the only really "wrong" thing is the docstring, but I think it does need fixing.

lib/iris/util.py Outdated Show resolved Hide resolved
lib/iris/util.py Outdated Show resolved Hide resolved
lib/iris/tests/unit/util/test_find_discontiguities.py Outdated Show resolved Hide resolved
lib/iris/tests/unit/util/test_find_discontiguities.py Outdated Show resolved Hide resolved
Copy link
Member

@pp-mo pp-mo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(sorry, additional point just spotted after I submitted the previous review)

lib/iris/tests/stock/_stock_2d_latlons.py Show resolved Hide resolved
@pp-mo pp-mo merged commit 6b4ba73 into SciTools:main Apr 12, 2023
tkknight added a commit to tkknight/iris that referenced this pull request Apr 13, 2023
* upstream/main: (59 commits)
  Updated environment lockfiles (SciTools#5211)
  update ci locks location (SciTools#5228)
  Fixes to _discontiguity_in_bounds (attempt 2) (SciTools#4975)
  Finalises Lazy Data documentation (SciTools#5137)
  Modernize and simplify iris.analysis._Groupby (SciTools#5015)
  clarity on whatsnew entry contributors (SciTools#5240)
  Handle derived coordinates correctly in `concatenate` (SciTools#5096)
  Use real array for data of of small netCDF variables. (SciTools#5229)
  Bump scitools/workflows from 2023.04.1 to 2023.04.2 (SciTools#5236)
  fixing whatsnew entry
  remove results creation commit from blame
  configure codecov
  adding a whatsnew entry
  Replacing numpy legacy printing with array2string and remaking results for dependent tests
  Adding a whatsnew entry for 5224 (SciTools#5234)
  Cf cell method (SciTools#5224)
  Bump scitools/workflows from 2023.03.3 to 2023.04.1 (SciTools#5231)
  [pre-commit.ci] pre-commit autoupdate (SciTools#5230)
  Bump scitools/workflows from 2023.03.2 to 2023.03.3 (SciTools#5227)
  raise dask min pin (SciTools#5225)
  ...
pp-mo pushed a commit to pp-mo/iris that referenced this pull request Apr 13, 2023
tkknight added a commit to tkknight/iris that referenced this pull request Apr 18, 2023
* upstream/main: (29 commits)
  review actions
  update .git-blame-ignore-revs
  adopt codespell
  Adopt sphinx design (SciTools#5127)
  Bump scitools/workflows from 2023.04.2 to 2023.04.3 (SciTools#5253)
  refresh manual pypi publish instructions (SciTools#5252)
  Updated environment lockfiles (SciTools#5250)
  removed bugfix section
  Make bm_runner location agnostic and include debugging. (SciTools#5247)
  Restore latest Whats New files.
  SciTools#5220 typo github.repository_owner. (SciTools#5248)
  Whats new updates for v3.5.0rc0. (SciTools#5246)
  libnetcdf <4.9 pin (SciTools#5242)
  update cf standard units (SciTools#5244)
  Updated environment lockfiles (SciTools#5211)
  update ci locks location (SciTools#5228)
  Fixes to _discontiguity_in_bounds (attempt 2) (SciTools#4975)
  Finalises Lazy Data documentation (SciTools#5137)
  Modernize and simplify iris.analysis._Groupby (SciTools#5015)
  clarity on whatsnew entry contributors (SciTools#5240)
  ...
lbdreyer pushed a commit that referenced this pull request Apr 21, 2023
* Basic functional lazy saving.

* Simplify function signature which upsets Sphinx.

* Non-lazy saves return nothing.

* Now fixed to enable use with process/distributed scheduling.

* Remove dask.utils.SerializableLock, which I think was a mistake.

* Make DefferedSaveWrapper use _thread_safe_nc.

* Fixes for non-lazy save.

* Avoid saver error when no deferred writes.

* Reorganise locking code, ready for shareable locks.

* Remove optional usage of 'filelock' for lazy saves.

* Document dask-specific locking; implement differently for threads or distributed schedulers.

* Minor fix for unit-tests.

* Pin libnetcdf to avoid problems -- see #5187.

* Minor test fix.

* Move DeferredSaveWrapper into _thread_safe_nc; replicate the NetCDFDataProxy fix; use one lock per Saver; add extra up-scaled test

* Update lib/iris/fileformats/netcdf/saver.py

Co-authored-by: Bouwe Andela <[email protected]>

* Update lib/iris/fileformats/netcdf/_dask_locks.py

Co-authored-by: Bouwe Andela <[email protected]>

* Update lib/iris/fileformats/netcdf/saver.py

Co-authored-by: Bouwe Andela <[email protected]>

* Small rename + reformat.

* Remove Saver lazy option; all lazy saves are delayed; factor out fillvalue checks and make them delayable.

* Repurposed 'test__FillValueMaskCheckAndStoreTarget' to 'test__data_fillvalue_check', since old class is gone.

* Disable (temporary) saver debug printouts.

* Fix test problems; Saver automatically completes to preserve existing direct usage (which is public API).

* Fix docstring error.

* Fix spurious error in old saver test.

* Fix Saver docstring.

* More robust exit for NetCDFWriteProxy operation.

* Fix doctests by making the Saver example functional.

* Improve docstrings; unify terminology; simplify non-lazy save call.

* Moved netcdf cell-method handling into nc_load_rules.helpers, and various tests into more specific test folders.

* Fix lockfiles and Makefile process.

* Add unit tests for routine _fillvalue_report().

* Remove debug-only code.

* Added tests for what the save function does with the 'compute' keyword.

* Fix mock-specific problems, small tidy.

* Restructure hierarchy of tests.unit.fileformats.netcdf

* Tidy test docstrings.

* Correct test import.

* Avoid incorrect checking of byte data, and a numpy deprecation warning.

* Alter parameter names to make test reports clearer.

* Test basic behaviour of _lazy_stream_data; make 'Saver._delayed_writes' private.

* Add integration tests, and distributed dependency.

* Docstring fixes.

* Documentation section and whatsnew entry.

* Various fixes to whatsnew, docstrings and docs.

* Minor review changes, fix doctest.

* Arrange tests + results to organise by package-name alone.

* Review changes.

* Review changes.

* Enhance tests + debug.

* Support scheduler type 'single-threaded'; allow retries on delayed-save test.

* Improve test.

* Adding a whatsnew entry for 5224 (#5234)

* Adding a whatsnew entry explaining 5224

* Fixing link and format error

* Replacing numpy legacy printing with array2string and remaking results for dependent tests

* adding a whatsnew entry

* configure codecov

* remove results creation commit from blame

* fixing whatsnew entry

* Bump scitools/workflows from 2023.04.1 to 2023.04.2 (#5236)

Bumps [scitools/workflows](https://github.com/scitools/workflows) from 2023.04.1 to 2023.04.2.
- [Release notes](https://github.com/scitools/workflows/releases)
- [Commits](SciTools/workflows@2023.04.1...2023.04.2)

---
updated-dependencies:
- dependency-name: scitools/workflows
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Use real array for data of of small netCDF variables. (#5229)

* Small netCDF variable data is real.

* Various test fixes.

* More test fixing.

* Fix printout in Mesh documentation.

* Whatsnew + doctests fix.

* Tweak whatsnew.

* Handle derived coordinates correctly in `concatenate` (#5096)

* First working prototype of concatenate that handels derived coordinates correctly

* Added checks for derived coord metadata during concatenation

* Added tests

* Fixed defaults

* Added what's new entry

* Optimized test coverage

* clarity on whatsnew entry contributors (#5240)

* Modernize and simplify iris.analysis._Groupby (#5015)

* Modernize and simplify _Groupby

* Rename variable to improve readability

Co-authored-by: Martin Yeo <[email protected]>

* Add a whatsnew entry

* Add a type hint to _add_shared_coord

* Add a test for iris.analysis._Groupby.__repr__

---------

Co-authored-by: Martin Yeo <[email protected]>

* Finalises Lazy Data documentation (#5137)

* cube and io lazy data notes added

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Added comments within analysis, as well as palette and iterate, and what's new

* fixed docstrings as requested in @trexfeathers review

* reverted cube.py for time being

* fixed flake8 issue

* Lazy data second batch

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* updated lastest what'snew

* I almost hope this wasn't the fix, I'm such a moron

* adressed review changes

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Bill Little <[email protected]>

* Fixes to _discontiguity_in_bounds (attempt 2) (#4975)

* update ci locks location (#5228)

* Updated environment lockfiles (#5211)

Co-authored-by: Lockfile bot <[email protected]>

* Increase retries.

* Change debug to show which elements failed.

* update cf standard units (#5244)

* update cf standard units

* added whatsnew entry

* Correct pull number

Co-authored-by: Martin Yeo <[email protected]>

---------

Co-authored-by: Martin Yeo <[email protected]>

* libnetcdf <4.9 pin (#5242)

* Pin libnetcdf<4.9 and update lock files.

* What's New entry.

* libnetcdf not available on PyPI.

* Fix for Pandas v2.0.

* Fix for Pandas v2.0.

* Avoid possible same-file crossover between tests.

* Ensure all-different testfiles; load all vars lazy.

* Revert changes to testing framework.

* Remove repeated line from requirements/py*.yml (?merge error), and re-fix lockfiles.

* Revert some more debug changes.

* Reorganise test for better code clarity.

* Use public 'Dataset.isopen()' instead of '._isopen'.

* Create output files in unique temporary directories.

* Tests for fileformats.netcdf._dask_locks.

* Fix attribution names.

* Fixed new py311 lockfile.

* Fix typos spotted by codespell.

* Add distributed test dep for python 3.11

* Fix lockfile for python 3.11

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: Bouwe Andela <[email protected]>
Co-authored-by: Henry Wright <[email protected]>
Co-authored-by: Henry Wright <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Manuel Schlund <[email protected]>
Co-authored-by: Bill Little <[email protected]>
Co-authored-by: Bouwe Andela <[email protected]>
Co-authored-by: Martin Yeo <[email protected]>
Co-authored-by: Elias <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: stephenworsley <[email protected]>
Co-authored-by: scitools-ci[bot] <107775138+scitools-ci[bot]@users.noreply.github.com>
Co-authored-by: Lockfile bot <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants