Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Stofs 2D Support #31

Merged
merged 2 commits into from
Jun 9, 2024
Merged

Stofs 2D Support #31

merged 2 commits into from
Jun 9, 2024

Conversation

mpiannucci
Copy link
Collaborator

@mpiannucci mpiannucci commented Jun 6, 2024

@omkar-334 Please test out this PR.

When I download the data locally, the subset takes 4.0s, but when using the s3 version it takes a minute or minute and a half.

I am happy to chat through these changes with you so you understand. The issue here is that the dimension ordering for the face node connectivity was reversed from the ordering that FVCOM uses, so we have to make the ugrid subsetter aware of this.

I looked at the comparison with thalassa and I believe the difference in speed has to do with the reindexing and the the element selection but i am not sure. But this should give results that are more comparable I believe.

#16
#9

@mpiannucci mpiannucci changed the title Stofs handle Stofs 2D Support Jun 6, 2024
@omkar-334
Copy link
Contributor

omkar-334 commented Jun 7, 2024

I looked at the comparison with thalassa and I believe the difference in speed has to do with the reindexing and the the element selection but i am not sure. But this should give results that are more comparable I believe.

Got it, I have tested this out and it does indeed give more comparable results.
Moreover, I noticed a considerable decrease in computation time when using fsspec rather than s3fs.

@@ -113,31 +112,54 @@ def subset_polygon(
x_var, y_var = mesh.node_coordinates.split(" ")
x, y = ds[x_var], ds[y_var]

# NOTE: When the first dimension is "nele", the face_node_connectivity
Copy link
Collaborator

Choose a reason for hiding this comment

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

According to the UGRID spec:
"""
The connectivity array will thus be a matrix of size nFaces x 3.
"""
so the "nele" dimension should always be first in a compliant file.

If we are finding files that. need to be transposed (ADCIRC?) -- what to do?

I think that should be handled by a pre-processor -- the make compliant function:

assign_ugrid_topology

If we are consistent about that, then we'll save a lot if blocks in the core code, like here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I do not think the preprocessor should mess with the dataset itself personally, but I can be convinced maybe.

Yes the ADCIRC files have reversed dimensions on the face node connectivity, which could happen with any dataset really.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You are probably right -- as much as I want to be pedantic about the standard, we shouldn't actually change anything (as opposed to adding metadata) in the process unless the user asks for it ...

@mpiannucci mpiannucci merged commit fbc5c34 into main Jun 9, 2024
9 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants