Skip to content

Conversation

mpiannucci
Copy link
Contributor

@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.

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
Contributor

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
Contributor 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
Contributor

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
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.

3 participants