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.

Sign up for GitHub

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

Make difference module handle circular cubes #1990

Closed
wants to merge 24 commits into from
Closed

Make difference module handle circular cubes #1990

Changes from 1 commit
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
50d08bb
Resolved merge conflict
MO-PeterJordan Apr 22, 2024
e4bce85
Added another failing test for difference module. Differences calcula…
MO-PeterJordan Apr 22, 2024
beabc9e
Resolved merge conflicts
MO-PeterJordan Apr 23, 2024
141ee1f
Auto-formatting
MO-PeterJordan Apr 23, 2024
9535d45
Import Bugfix
MO-PeterJordan Apr 23, 2024
c442023
Ran isort against spatial.py
MO-PeterJordan Apr 23, 2024
62dc4d9
Added test for appropriate error for unhandled cubes and made code ma…
MO-PeterJordan Apr 23, 2024
636ec69
iSort on spatial.py again
MO-PeterJordan Apr 23, 2024
fa2b8b0
Another attempt at autoformatting
MO-PeterJordan Apr 23, 2024
53659a6
Tidied up
MO-PeterJordan Apr 23, 2024
617bd27
Removed confusing variable name
MO-PeterJordan Apr 24, 2024
0910cd6
Autoformatting
MO-PeterJordan Apr 24, 2024
8082dd0
Tried manually sorting import order as isort still failing in github …
MO-PeterJordan Apr 24, 2024
22e920a
And again
MO-PeterJordan Apr 24, 2024
ce71d4c
And again
MO-PeterJordan Apr 24, 2024
b74833b
Linting suggestion
MO-PeterJordan Apr 24, 2024
ccdc67f
Review Actions
MO-PeterJordan May 20, 2024
c51ad05
Ran isort to fix gha
MO-PeterJordan May 23, 2024
8eb9195
Added test for cubes with flipped axes (lonitude increasing along col…
MO-PeterJordan Jun 17, 2024
939ac46
Merge branch 'master' into pjordan-make-difference-module-handle-circ…
MO-PeterJordan Jun 17, 2024
e3496eb
black, isort, flake8
MO-PeterJordan Jun 17, 2024
175e825
Ran isort in isolation
MO-PeterJordan Jun 17, 2024
6fbc3eb
Fixed test failures resulting from breaking changes made by upstream PR
MO-PeterJordan Jun 17, 2024
31fceb3
Black
MO-PeterJordan Jun 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Removed confusing variable name
  • Loading branch information
@MO-PeterJordan
MO-PeterJordan committed Apr 24, 2024
commit 617bd2769cd74eb5f374a39b00b2bcf7c45b7774
3 changes: 1 addition & 2 deletions improver/utilities/spatial.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,9 +243,8 @@ def create_difference_cube(
"DifferenceBetweenAdjacentGridSquares does not currently support cubes with "
"circular x-axis that do not use a geographic (i.e. latlon) coordinate system."
)
max_value = 360
extra_mean_point = (
np.mean([points[-1], (points[0] + max_value)]) % max_value
np.mean([points[-1], (points[0] + 360)]) % 360
)
mean_points = np.hstack([mean_points, extra_mean_point])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Question for the reviewer:

This will only work if the 'wrap around' point is positive. This is the case for test cubes that are 'symmetric' in nature (eg. points in cube at [120, 0, 120] or [-175, 165, 155, ..., -25, -15, -5, 5, 15, 25, ..., 165, 175] - wrap around point = 180) but not ones that are 'offset' (eg. [-170, -160, -150, ..., -20, -10, 0, 10, 20, ..., 150, 160, 170, 180] - wrap around point = -175).

In the offset case given above, this would calculate the 'wrap around point' to be 185. Not 'wrong' per-se but certainly potentially confusing.

I could add extra logic to make this work for offset grids. However given that global data from STaGE is 'symmetrical', and the cubes produced by the set_up_variable_cube test helper match this convention, I'm not sure whether or not this is something worth investing time in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

...Thinking about it, for the symmetrical case above, the np.mean(...) line always resolves to 180, so I could get rid of it altogether and just always put the wrap around point at 180.

The above does better handle the case where the cube uses 0 -> 360 instead of -180 -> 180 but maybe this never comes up.

I suppose the line could also be used as a test. I.e. if np.mean(...) != 180 raise ValueError("Difference Plugin only handles circular cubes if their points are spread symmetrically about a plane intersecting the prime meridian").

Copy link
Contributor

Choose a reason for hiding this comment

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

There's a nice function I knew existed somewhere for calculating the circular mean. It's in scipy https://docs.scipy.org/doc/scipy/reference/generated/scipy.stats.circmean.html . This might be helpful but it does still need to be told the bounds to know where to wrap around so I don't know if it solves every problem but should simplify the offset case.

Id keep this function as general as possible. so I wouldn't raise an error, at worst a warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have split this calculation out into its own function which now uses circmean from scipy.


Expand Down
Loading