Make gradient consider lat-lon distances as metres #2017
Merged
Conversation
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
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2017 +/- ##
==========================================
- Coverage 98.39% 98.27% -0.13%
==========================================
Files 124 131 +7
Lines 12212 12752 +540
==========================================
+ Hits 12016 12532 +516
- Misses 196 220 +24 ☔ View full report in Codecov by Sentry. |
|
I've had a go at getting this PR done. It still needs more work in the test_GradientBetweenAdjacentGridSquares module (see todos). |
|
TODO: I don't think I've written any tests for the |
mspelman07
requested changes
Aug 1, 2024
improver_tests/utilities/spatial/test_GradientBetweenAdjacentGridSquares.py
Show resolved
Hide resolved
I can't fix the CONTRIBUTING error. It comes from Peter's credentials. I'm hoping he remembers how he got around this last time. |
mspelman07
previously approved these changes
Aug 2, 2024
I've (hopefully) fixed this on a local copy of the branch. However, as it's now in Stephen's Improver fork, I don't have permission to push it to this PR. |
bayliffe
requested changes
Aug 8, 2024
improver_tests/utilities/spatial/test_DistanceBetweenGridSquares.py
Outdated
Show resolved
Hide resolved
improver_tests/utilities/spatial/test_DistanceBetweenGridSquares.py
Outdated
Show resolved
Hide resolved
improver_tests/utilities/spatial/test_DistanceBetweenGridSquares.py
Outdated
Show resolved
Hide resolved
improver_tests/utilities/spatial/test_DistanceBetweenGridSquares.py
Outdated
Show resolved
Hide resolved
improver_tests/utilities/spatial/test_DistanceBetweenGridSquares.py
Outdated
Show resolved
Hide resolved
926c626 to
3e07726
Compare
|
@MO-PeterJordan - I've rebased my branch on your one, and I seem to have picked up a load of conflicts. Any idea why? |
3e07726 to
22bc899
Compare
|
I've reverted my rebase (thanks to |
d47f206 to
eabb014
Compare
Made input test fixture configurable Got test for gradient on lat/long grid failing for correct reasons (response is in m/s/degree; needs to be /s). Commented out regridding test for now. Hacked about and more or less got latlon distance calculation working but made a lot of mess. Plan to abstract it to its own class so I can test it properly and allow it to be reused. First test for distance class fails in the right way Got happy path test passing Added more tests. Did some tidying Prepared to implement equalarea logic. Wrote failing test. Test for equalarea passes Tidying up Gradient test passes Improved tests. Nearly working. Problem with generating expected data for y when interpolated back out to original dims. Added extra test cases and got all tests working. Reduced required absolute precision due to unexplained weirdness in how iris linear interpolation works when regridding with large gradients. Tidied up tests Ran black against test file then re-squared the arrays Tidying Got non-standard units working for equal area projections. Not going to implement for latlong projections, as anything other than degrees is rare, and will not fail silently in any case Removed unused test Moderately graceless exception handling. Open to suggestions for a better way to work out which coordinates represent latitude and longitude. Fixed cube instantiation to allow better way of checking projection type More tidying Tidied imports in spatial utilities Review comments - Tuple type hinting fix Ran autoformatting tools Added docstrings for some methods Moved static method back to original location Added docstrings to all of the new functions in spatial.py Added some more docstrings to test helper functions Review actions around error handling Numpy efficiency improvement Review actions and tidying Added extra tests and made distance module handle all coordinate systems that provide distances between grid points Added a todo comment. Will be actioned shortly spelling correction Co-authored-by: Stephen Moseley <stephen.moseley@metoffice.gov.uk> more spelling corrections Co-authored-by: Stephen Moseley <stephen.moseley@metoffice.gov.uk> Yet more spelling corrections Co-authored-by: Stephen Moseley <stephen.moseley@metoffice.gov.uk> And another Co-authored-by: Stephen Moseley <stephen.moseley@metoffice.gov.uk> Spelling corrections: Meters -> Metres Switched to using earth radius supplied with input cube rather than hard-coded constant Added docstrings to distance tests Review actions Co-authored-by: Stephen Moseley <stephen.moseley@metoffice.gov.uk> tidied up y latlon distances method Tidied up x distances calculation Arguably simplified latlon distance methods. Going to try to split out further using factory pattern. Will revert if it just makes things worse. Tests passing after factory refactor. Still needs lots of tidying. Finished tidying A little more tidying Review actoin Updated dosctring and deleted corresponding one on child class as sphinx handles docstring inheritence Fixed gradient module broken by refactor of distance module Docstring change. Ran autoformatting Added appropriate decoration to distances base class Added failing tests to gradient module for wrapping around the meridian - still need to move grid points used in tests to cover whole globe at 120 degree increments. Made test cube generation work with non-equal x and y spacing Update improver/utilities/spatial.py Co-authored-by: Stephen Moseley <stephen.moseley@metoffice.gov.uk> Docstring improvements Working on gradient tests. They seem to only work for 10 degrees separations. Expected values are correct, actuals wrong. Amended test data to give gradients large enough that no tests pass due to both expected and actual being too small to tell the difference between them Fixed bug causing tests to fail for non wrap-around case. Spelling corrections Updated docstring Switched to 120 degree longitude separations. Some tests now failing. Expected values correct; mid way through debugging actuals. Got part way to testing wrap-around interpolaiton. Found a bug with non wrap-around calculations for large angles along the way, and got part way through fixing it Added tests for different longitude separations to distance module and fixed some bugs. Added failing tests to Gradient module for circular latlong grid More faffing with tests Amalgamated some distance tests into single parameterised function. Fixed testing bug More test tidying Distance tests fail correctly for circular cubes Started work to make distance caluclation work for circular coords Removed dependency on diffs from projection cube subclass. Did some tidying. Made gradient stop passing diffs to distances method Added test for projected cubes which wrap around the meridian Made difference module handle cubes that wrap around the meridian Added another failing test for difference module. Differences calculated correctly but need to add extra dimension for the wrap-around distance. Driveby docstring fix Removed some completed todos. Added a couple of new ones Removed another completed tod Got distance module passing test for circualar coords Make difference module handle circular cubes (metoppv#2016) * Resolved merge conflict * Added another failing test for difference module. Differences calculated correctly but need to add extra dimension for the wrap-around distance. * Resolved merge conflicts * Auto-formatting * Import Bugfix * Ran isort against spatial.py * Added test for appropriate error for unhandled cubes and made code match it. * iSort on spatial.py again * Another attempt at autoformatting * Tidied up * Removed confusing variable name * Autoformatting * Tried manually sorting import order as isort still failing in github actions despite running locally and pushing * And again * And again * Linting suggestion * Review Actions * Ran isort to fix gha * Added test for cubes with flipped axes (lonitude increasing along columns not rows and vice versa for latitude). Fixed code to handle this case. * black, isort, flake8 * Ran isort in isolation * Fixed test failures resulting from breaking changes made by upstream PR * Black * updates as per review comments and adds test for 0-360 degree case * update comment * formatting --------- Co-authored-by: Marcus Spelman <marcus.spelman@metoffice.gov.uk>
- circular projections raise error - make_test_cube uses set_up_variable_cube - black
- Tests non-uniform lat-lon grids
* Resolved merge conflict * Added another failing test for difference module. Differences calculated correctly but need to add extra dimension for the wrap-around distance. * Resolved merge conflicts * Auto-formatting * Import Bugfix * Ran isort against spatial.py * Added test for appropriate error for unhandled cubes and made code match it. * iSort on spatial.py again * Another attempt at autoformatting * Tidied up * Removed confusing variable name * Autoformatting * Tried manually sorting import order as isort still failing in github actions despite running locally and pushing * And again * And again * Linting suggestion * Review Actions * Ran isort to fix gha * Added test for cubes with flipped axes (lonitude increasing along columns not rows and vice versa for latitude). Fixed code to handle this case. * black, isort, flake8 * Ran isort in isolation * Fixed test failures resulting from breaking changes made by upstream PR * Black * updates as per review comments and adds test for 0-360 degree case * update comment * formatting --------- Co-authored-by: Marcus Spelman <marcus.spelman@metoffice.gov.uk>
* Add apply_mask cli and function * remove print statement * Updates to make sure plugin doesn't require cubelist input * Update Docstring --------- Co-authored-by: Marcus Spelman <marcus.spelman@metoffice.gov.uk>
* Adds masked_add to cube combiner * Remove unnecessary import * formatting * add docstrings and simplifications * formatting --------- Co-authored-by: Marcus Spelman <marcus.spelman@metoffice.gov.uk>
Pulling over pinned packages introduced to the `conda-forge.yml` into the `latest.yml` file. metoppv#2011
* refactored simple_bias_correction cli * docstring and contributing * changes for actions * sphinx building locally without error * whitespace change * bugfix: test upper_bound * changes based on review * review changes * review - doc-strings
* Plugin for calculating the height of the maximum vertical velocity once the maximum vertical velocity has been calculated. * Minor code updates. * Deleting or adding required spaces. * Updating some comments. * Adds masked_add to cube combiner * Remove unnecessary import * formatting * Updating and improving unit tests, and adding cli. * Isort changes. * Formatting changes. * Sorting out trailing whitespaces. * Sorting out formatting. * flake8 change. * Further changes to get acceptance tests to work. * Changes to get acceptance test data to run. * add docstrings and simplifications * formatting * update checksums and correct acceptance test bug * Update default name and change low_or_high to boolean * updates acceptance tests * formatting --------- Co-authored-by: Kathryn.Howard <kathryn.howard@metoffice.gov.uk> Co-authored-by: Marcus Spelman <marcus.spelman@metoffice.gov.uk>
* Resolved merge conflict * Added another failing test for difference module. Differences calculated correctly but need to add extra dimension for the wrap-around distance. * Resolved merge conflicts * Auto-formatting * Import Bugfix * Ran isort against spatial.py * Added test for appropriate error for unhandled cubes and made code match it. * iSort on spatial.py again * Another attempt at autoformatting * Tidied up * Removed confusing variable name * Autoformatting * Tried manually sorting import order as isort still failing in github actions despite running locally and pushing * And again * And again * Linting suggestion * Review Actions * Ran isort to fix gha * Added test for cubes with flipped axes (lonitude increasing along columns not rows and vice versa for latitude). Fixed code to handle this case. * black, isort, flake8 * Ran isort in isolation * Fixed test failures resulting from breaking changes made by upstream PR * Black * updates as per review comments and adds test for 0-360 degree case * update comment * formatting --------- Co-authored-by: Marcus Spelman <marcus.spelman@metoffice.gov.uk>
…distance * refs/heads/master: Height of max (metoppv#2020) Migrate CLI functionality: simple_bias_correction (metoppv#2018) BUG: Fix Scheduled Tests Sphinx-Pytest-Coverage (metoppv#2021) Plugin discovery (metoppv#2009) Adds masked_add to cube combiner (metoppv#2015) Apply mask to cube (metoppv#2014) Make difference module handle circular cubes (metoppv#2016) # Conflicts: # improver/utilities/spatial.py # improver_tests/utilities/spatial/test_DifferenceBetweenAdjacentGridSquares.py
04b893e to
4d23759
Compare
|
Ok. I'm pretty confident that this PR now contains the changes that were originally reviewed, and no others (except for merges from master), so I believe it is ready to be reviewed again. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Replaces #1985 with this version that is merged with
master.This is part of the EPP Relative Vorticity work. https://metoffice.atlassian.net/browse/EPPT-825
For lat-lon grids, the existing gradient module does not divide through by the distance between grid points. Instead it divides through by the angular separation, resulting in units of original-units * deg^-1. This PR changes that to divide through by distance in metres, so that the resulting units are original-units * m-1. This is what we would naturally expect, and allows eg. relative vorticity to be calculated with the correct units of s^-1.
Tests for the
utilities/spatial.pymodule have been moved intotests/utilities/spatial/, the convention followed elsewhere in this repo.I have tweaked the difference plugin too so that the resulting coordinates are calculated in the same way.
Potential issues:
When calculating the gradient of wind speed (m/s) Iris presents the resulting units as Hz, which might be confusing for a gradient. However, grad_cube.units == s^1 returns true, so iris' units logic for downstream applications will be fine.
Absolute precision requirement has been reduced in the tests from e-8 to e-4 as this threshold was being breached in cases with very large gradients (>6 orders of magnitude between adjacent grid cells) where interpolation where required to recover the original grid shape. This seems to be to do with the way iris does linear interpolation. However O(10^6) gradients between adjacent cells seem unlikely to occur for meteorological parameters. Furthermore, +/-0.0001 should be enough precision for gradients of most if not all parameters.
Testing: