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

Merge Duplicate Node Indices #879

Open
wants to merge 7 commits into
base: zedwick/dual_mesh
Choose a base branch
from

Conversation

philipc2
Copy link
Member

@philipc2 philipc2 commented Aug 6, 2024

Closes #865

Overview

  • Adds the merge_duplicate_node_indices() function that replaces the index of nodes that have the exact latitude and longitude to only reference one node.

Expected Usage

import uxarray as ux

grid_path = "/path/to/grid.nc"

uxgrid = ux.open_grid(grid_path)

# inplace
uxgrid.merge_duplicate_node_indices()

# return a new grid
new_grid = uxgrid.merge_duplicate_node_indices(inplace=False)

@philipc2 philipc2 self-assigned this Aug 6, 2024
@philipc2 philipc2 linked an issue Aug 6, 2024 that may be closed by this pull request
@philipc2 philipc2 changed the title DRAFT: Merge Duplicate Node Indices Merge Duplicate Node Indices Aug 6, 2024
@philipc2 philipc2 requested a review from rajeeja August 6, 2024 18:08
@philipc2 philipc2 marked this pull request as ready for review August 6, 2024 18:13
@aaronzedwick
Copy link
Member

Do you know what the problem was with the previous implementation? If I remember it was still having problems?

@philipc2 philipc2 mentioned this pull request Aug 12, 2024
10 tasks
@philipc2
Copy link
Member Author

Do you know what the problem was with the previous implementation? If I remember it was still having problems?

I haven't been able to pinpoint the problem, but everything appears to be working correctly here according to the tests.

@philipc2 philipc2 changed the base branch from main to zedwick/dual_mesh August 12, 2024 20:53
@philipc2
Copy link
Member Author

@aaronzedwick

I'm going to merge these changes into the branch used in #859 to see the compatibility.

@philipc2
Copy link
Member Author

@aaronzedwick

May you take a look at the case using the geos dataset? Looks like something is breaking in the construct_faces function call.

@rajeeja
Copy link
Contributor

rajeeja commented Aug 12, 2024

what tests are failing? we may want to issue a warning on how and what is supported.
Supporting all formats may be harder and might break with corner cases.

@philipc2
Copy link
Member Author

what tests are failing? we may want to issue a warning on how and what is supported. Supporting all formats may be harder and might break with corner cases.

Something with the Dual Mesh Construction on the Cube Sphere grid is breaking. It's complaining about a dimension miss match when assigned the faces.

I don't believe it's an issue with how we are merging ndoes.

@aaronzedwick
Copy link
Member

@aaronzedwick

May you take a look at the case using the geos dataset? Looks like something is breaking in the construct_faces function call.

Oh yeah, I know the problem. Let me fix it for you.

@aaronzedwick
Copy link
Member

@philipc2 There you go!

@philipc2
Copy link
Member Author

@philipc2 There you go!

Thanks for fixing this so quickly!

@philipc2
Copy link
Member Author

@aaronzedwick

There still appear to be some underlying issues, at least on the Cube Sphere grid.

Running the following executes, however when I try to plot it I get the following error:

uxgrid.merge_duplicate_node_indices(inplace=True)
dual = uxgrid.compute_dual()

IndexError Traceback (most recent call last)
Cell In[18], line 1
----> 1 dual.plot()

File ~/anaconda3/envs/dev-uxarray/lib/python3.11/site-packages/uxarray/plot/accessor.py:30, in GridPlotAccessor.call(self, **kwargs)
29 def call(self, **kwargs) -> Any:
---> 30 return grid_plot.plot(self._uxgrid, **kwargs)

File ~/anaconda3/envs/dev-uxarray/lib/python3.11/site-packages/uxarray/plot/grid_plot.py:16, in plot(grid, **kwargs)
14 def plot(grid: Grid, **kwargs):
15 """Default Plotting Method for Grid."""
---> 16 return mesh(grid, **kwargs)

File ~/anaconda3/envs/dev-uxarray/lib/python3.11/site-packages/uxarray/plot/grid_plot.py:43, in mesh(uxgrid, backend, exclude_antimeridian, width, height, xlabel, ylabel, **kwargs)
19 def mesh(
20 uxgrid: Grid,
21 backend: Optional[str] = "bokeh",
(...)
27 **kwargs,
28 ):
29 """Vector Line Plot of the edges that make up each face.
30
31 Parameters
(...)
40 Plot Width for Bokeh Backend
41 """
---> 43 gdf = uxgrid.to_geodataframe(exclude_antimeridian=exclude_antimeridian)
45 hv_paths = hv.Path(gdf)
47 uxarray.plot.utils.backend.assign(backend=backend)

File ~/anaconda3/envs/dev-uxarray/lib/python3.11/site-packages/uxarray/grid/grid.py:1304, in Grid.to_geodataframe(self, override, cache, exclude_antimeridian)
1301 return self._gdf
1303 # construct a geodataframe with the faces stored as polygons as the geometry
-> 1304 gdf = _grid_to_polygon_geodataframe(
1305 self, exclude_antimeridian=exclude_antimeridian
1306 )
1308 # cache computed geodataframe
1309 if cache:

File ~/anaconda3/envs/dev-uxarray/lib/python3.11/site-packages/uxarray/grid/geometry.py:95, in _grid_to_polygon_geodataframe(grid, exclude_antimeridian)
91 def _grid_to_polygon_geodataframe(grid, exclude_antimeridian):
92 """Converts the faces of a Grid into a spatialpandas.GeoDataFrame
93 with a geometry column of polygons."""
---> 95 polygon_shells = _build_polygon_shells(
96 grid.node_lon.values,
97 grid.node_lat.values,
98 grid.face_node_connectivity.values,
99 grid.n_face,
100 grid.n_max_face_nodes,
101 grid.n_nodes_per_face.values,
102 )
104 antimeridian_face_indices = _build_antimeridian_face_indices(
105 polygon_shells[:, :, 0]
106 )
108 if grid.n_face > GDF_POLYGON_THRESHOLD:

File ~/anaconda3/envs/dev-uxarray/lib/python3.11/site-packages/uxarray/grid/geometry.py:82, in _build_polygon_shells(node_lon, node_lat, face_node_connectivity, n_face, n_max_face_nodes, n_nodes_per_face, projection)
77 node_lon = lonlat_proj[:, 0]
78 node_lat = lonlat_proj[:, 1]
80 polygon_shells = (
81 np.array(
---> 82 [node_lon[closed_face_nodes], node_lat[closed_face_nodes]], dtype=np.float32
83 )
84 .swapaxes(0, 1)
85 .swapaxes(1, 2)
86 )
88 return polygon_shells

IndexError: index -9223372036854775808 is out of bounds for axis 0 with size 864

Some of the entries in the face_node_connectivity are now incorrect. There are some entries that are an entire row of fill values.

The number of entirely zero entries corresponds to how many duplicates were found.

image

Instead of having rows of full fill values, we should ensure that each duplicate has the same entry in the connectivity.

@aaronzedwick
Copy link
Member

@philipc2 I am not exactly sure what's happening, but I don't believe its a problem with my code. The only reason you are getting that error is because the connectivity that you are passing in is broken I think. Comment out the line in construct_faces that says if n_edges[i] < 3: continue and then compute dual will run, and the plot is still incorrect. All those fill value entries look to only have one node for some reason around which it is trying to make a dual face. Something with the node_face_connectivity I suppose?

@philipc2
Copy link
Member Author

@philipc2 I am not exactly sure what's happening, but I don't believe its a problem with my code. The only reason you are getting that error is because the connectivity that you are passing in is broken I think. Comment out the line in construct_faces that says if n_edges[i] < 3: continue and then compute dual will run, and the plot is still incorrect. All those fill value entries look to only have one node for some reason around which it is trying to make a dual face. Something with the node_face_connectivity I suppose?

Yeah that seems to be the case, I'll look into it!

@philipc2
Copy link
Member Author

@philipc2 I am not exactly sure what's happening, but I don't believe its a problem with my code. The only reason you are getting that error is because the connectivity that you are passing in is broken I think. Comment out the line in construct_faces that says if n_edges[i] < 3: continue and then compute dual will run, and the plot is still incorrect. All those fill value entries look to only have one node for some reason around which it is trying to make a dual face. Something with the node_face_connectivity I suppose?

Something still appears to be off, I'm getting the same error now (even though the connectivity appears to have been corrected)

image

@aaronzedwick
Copy link
Member

Can you push the changes that fixed the connectivity?

Copy link
Contributor

@rajeeja rajeeja left a comment

Choose a reason for hiding this comment

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

just one minor test and this is good go.


def test_duplicate(self):
uxgrid = ux.open_grid(gridfile_geos)
uxgrid.merge_duplicate_node_indices()
Copy link
Contributor

Choose a reason for hiding this comment

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

add an assert to check that compute_dual fails or produces incorrect stuff without this operation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Handling Duplicate Node Indices
3 participants