Skip to content

2D coord bounds handling #3401

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

Closed
stephenworsley opened this issue Sep 16, 2019 · 10 comments
Closed

2D coord bounds handling #3401

stephenworsley opened this issue Sep 16, 2019 · 10 comments

Comments

@stephenworsley
Copy link
Contributor

stephenworsley commented Sep 16, 2019

Currently, a 2D coordinate point is expected to have 4 bounds. There ought to be 8. Assuming the 2D variable is defined over lattitude and longitude, there should be 4 latbounds and 4 lonbounds. http://cfconventions.org/Data/cf-conventions/cf-conventions-1.7/cf-conventions.html#cell-boundaries
The current way these bounds are handled is incomplete. _discontiguity_in_bounds should be making more comparisons. It is currently comparing bounds[*,*,0] with bounds[*,*,1] and bounds[*,*,0] with bounds[*,*,3]. it should also be comparing bounds[*,*,1] with bounds[*,*,2] and bounds[*,*,2] with bounds[*,*,3]. These comparisons should be done for both latbounds and lonbounds.

It's also worth noting that it's possible to end up with a coordinate which has bounds in one dimension but not another. This is in fact the case for the gallery example https://scitools.org.uk/iris/docs/v1.9.2/examples/General/cross_section.html . It's unclear how such cases should be treated (they are currently unable to be handled by pcolor/pcolormesh. That is perhaps the correct approach, but in order for something like that to be possible, it seems like there would need to be some more thought put into the structure of bounds handling).

@stephenworsley
Copy link
Contributor Author

My intuition is to have the shape of 2D bounds be (*,*,2,4). I've taken a quick look at where in the code may have to change and I came up with the following list of functions:
coords._get_2d_coord_bound_grid
coords.Cell.contains_point
coords.Coord._bounds_setter
coords.Coord._sanity_check_bounds
coords.Coord._discontiguity_in_bounds
coords.Coord.contiguous_bounds
coords.Coord.nbounds
coords.Coord._guess_bounds
coords.DimCoord._new_bounds_requirements
analysis._grid_angles

And potentially a bunch of places with load/save rules.

@stephenworsley
Copy link
Contributor Author

My intuition is to have the shape of 2D bounds be (*,*,2,4).

Having re-read the CF Conventions, I've reconsidered my position on this. It seems like CF Conventions is consistent with what we are doing currently. With that said, it still seems like we are not comparing enough points for discontiguity.
I'm still unsure how to interpret CF Conventions for derived coodinates or how 2D latlon grid bounds should behave when sliced.

@stephenworsley
Copy link
Contributor Author

Another concern I have about _discontiguity_in_bounds() is that the line

points_close_enough = diffs_along_axis <= (atol +

seems to be inappropriately comparing a boolean array to a float. I feel like it would make more sense to either replace diffs_along_axis with diffs_between_cells or else remove this line and replace the next line with

contiguous_along_axis = not np.any(diffs_along_axis)

@stephenworsley
Copy link
Contributor Author

Also, there seems to be some inconsistency as to what is returned by _discontiguity_in_bounds() as diffs. For 1D coordinates it seems to be returning an array of floats while for 2D bounds it is returning an array of booleans. It seems like wherever _discontiguity_in_bounds() is called, it is expecting diffs to contain booleans.

@stephenworsley
Copy link
Contributor Author

I have suggested a fix to everything I currently think is an issue in #3404.

@pp-mo
Copy link
Member

pp-mo commented Oct 4, 2019

inappropriately comparing a boolean array to a float.

Definitely a bug. It looks very much like a change to the implementation but then failing to delete a line of the previous version !

Thanks for making the effort here..

@cpelley
Copy link

cpelley commented Jan 22, 2020

See #3345

@trexfeathers trexfeathers assigned stephenworsley and unassigned pp-mo Jul 27, 2022
@trexfeathers
Copy link
Contributor

@stephenworsley has confirmed that there is still a problem here, but would like to describe it better before discussing further.

@stephenworsley
Copy link
Contributor Author

I think this issue has got garbled somewhat by my initial observation being mistaken, but my additional observations still (I think) being valid. There are two problems I've identified with 2D contiguous bounds checking I've found:

  • Not enough comparisons are being made.
    Currently, the only comparisons done are between the vertices indexed at 0 and 1 and the vertices indexed at 0 and 3. i.e. only the following comparisons are made:
3---2   3---2
|   |   |   |
0---1 + 0---1
+
3---2
|   |
0---1

In order to do a full comparison, we would also need to compare the vertices indexed at 1 and 2 and the vertices indexed at 2 and 3:

3---2 + 3---2
|   |   |   |
0---1 + 0---1
+   +
3---2
|   |
0---1
  • The logic involved in processing the diffs is nonsensical. Specifically this line:
    points_close_enough = diffs_along_axis <= (atol +

Both of these are addressed by #3404 which has currently stalled.

@stephenworsley
Copy link
Contributor Author

closed by #4975

@scitools-ci scitools-ci bot removed this from 🚴 Peloton Jul 14, 2023
@scitools-ci scitools-ci bot removed this from 🚴 Peloton Jul 15, 2023
@scitools-ci scitools-ci bot removed this from 🚴 Peloton Jul 16, 2023
@scitools-ci scitools-ci bot removed this from 🚴 Peloton Jul 17, 2023
@scitools-ci scitools-ci bot removed this from 🚴 Peloton Jul 18, 2023
@scitools-ci scitools-ci bot removed this from 🚴 Peloton Jul 19, 2023
@scitools-ci scitools-ci bot removed this from 🚴 Peloton Jul 20, 2023
@scitools-ci scitools-ci bot removed this from 🚴 Peloton Jul 21, 2023
@scitools-ci scitools-ci bot removed this from 🚴 Peloton Jul 22, 2023
@scitools-ci scitools-ci bot removed this from 🚴 Peloton Jul 23, 2023
@scitools-ci scitools-ci bot removed this from 🚴 Peloton Jul 24, 2023
@scitools-ci scitools-ci bot removed this from 🚴 Peloton Jul 25, 2023
@scitools-ci scitools-ci bot removed this from 🚴 Peloton Jul 26, 2023
@scitools-ci scitools-ci bot removed this from 🚴 Peloton Aug 2, 2023
@scitools-ci scitools-ci bot removed this from 🚴 Peloton Aug 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants