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

Read UGRID files with transposed connectivity (i.e. fesom.mesh.diag) #1025

Merged
merged 5 commits into from
Oct 23, 2024

Conversation

philipc2
Copy link
Member

@philipc2 philipc2 commented Oct 18, 2024

Closes #282 #1026

Overview

  • Updates UGRID reader to correctly parse grid files that contain transposed connectivity information
    • The fesom.mesh.diag.nc grid file stores connectivity information in a transpose order ([3, n_face] instead of [n_face, 3]
  • Fixes a bug where the start index was not correctly adjusted due to a negative fill value.

Plot of fesom.mesh.diag.nc

image

@philipc2 philipc2 self-assigned this Oct 18, 2024
@philipc2 philipc2 linked an issue Oct 18, 2024 that may be closed by this pull request
@philipc2 philipc2 added this to the UGRID Conventions milestone Oct 18, 2024
@philipc2 philipc2 requested a review from erogluorhan October 18, 2024 01:48
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.

Good fix, FESOM stores elements each node is connected to aka adjacency and happy to see that we had the diag file in main for over a year. It wasn't tested.
test/grid_geoflow.exo is removed ? It shouldn't have been there in the first place. Anyways. I'll approve as this will enable newer changes.

@philipc2
Copy link
Member Author

Good fix, FESOM stores elements each node is connected to aka adjacency and happy to see that we had the diag file in main for over a year. It wasn't tested. test/grid_geoflow.exo is removed ? It shouldn't have been there in the first place. Anyways. I'll approve as this will enable newer changes.

Yeah, one of our testing scrips produces the .exo file and it must of been included in a previous commit.

@rajeeja
Copy link
Contributor

rajeeja commented Oct 22, 2024

Good fix, FESOM stores elements each node is connected to aka adjacency and happy to see that we had the diag file in main for over a year. It wasn't tested. test/grid_geoflow.exo is removed ? It shouldn't have been there in the first place. Anyways. I'll approve as this will enable newer changes.

Yeah, one of our testing scrips produces the .exo file and it must of been included in a previous commit.

One minor thing that could make it good would be to have something about transpose in the README

@philipc2
Copy link
Member Author

Good fix, FESOM stores elements each node is connected to aka adjacency and happy to see that we had the diag file in main for over a year. It wasn't tested. test/grid_geoflow.exo is removed ? It shouldn't have been there in the first place. Anyways. I'll approve as this will enable newer changes.

Yeah, one of our testing scrips produces the .exo file and it must of been included in a previous commit.

One minor thing that could make it good would be to have something about transpose in the README

Good idea. I'll add a note to it.

@philipc2 philipc2 merged commit fb632d3 into main Oct 23, 2024
19 of 20 checks passed
@philipc2
Copy link
Member Author

@fluidnumerics-joe

I'm not sure if you've seen this PR, but we've already added and released support for reading the UGRID-compliant FESOM grid files.

I haven't followed up with #1013 since merging this. However, I'll work on getting it in soon.

@fluidnumerics-joe
Copy link
Collaborator

Sounds good. I haven't been able to get uxarray working for fesom2 netcdf grids from the latest pypi release, but I can from your philipc2/fesom branch. Curious to know the timeline to have these features in the pypi index .

@philipc2
Copy link
Member Author

Sounds good. I haven't been able to get uxarray working for fesom2 netcdf grids from the latest pypi release, but I can from your philipc2/fesom branch. Curious to know the timeline to have these features in the pypi index .

I'm going to wrap up #1013 this week and aim to release it next week.

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.

Support reading FESOM2 Mesh Diagnostic files FESOM grid connectivity stored as tranpose
4 participants