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

Design document for mesh class #38

Merged

Conversation

sbrus89
Copy link

@sbrus89 sbrus89 commented Dec 6, 2023

This design document details considerations for the data structures and functions concerning the mesh description in OMEGA

Checklist

  • Documentation:
    • Design document has been generated and added to the docs
    • Documentation has been built locally and changes look as expected

@sbrus89
Copy link
Author

sbrus89 commented Dec 6, 2023

This is a work in progress PR, but comments are welcome along the way.

Copy link

@philipwjones philipwjones left a comment

Choose a reason for hiding this comment

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

This is a good start - made a few comments here that mostly inform the design for later sections, so you can decide where to address. I'll wait to approve until the design portion is finished.

@@ -0,0 +1,73 @@
(omega-design-mesh)=
# Mesh

Choose a reason for hiding this comment

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

Do we want to call this HorzMesh since it only covers horizontal vars? Or do we want this to eventually include the vertical mesh vars too?

Copy link
Author

Choose a reason for hiding this comment

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

Good question. I like the idea of having a separate class for the vertical mesh, but would be curious to see what others think too.

Copy link

Choose a reason for hiding this comment

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

I would prefer this be for the horizontal mesh only, and be named accordingly. It seems like the vertical mesh will be pretty dynamic in some situations (e.g. not just layerThickness changing with time but also minLevelCell, maxLevelCell, etc.) so I would anticipate the vertical mesh being handled more like other prognostic variables.

Copy link
Author

Choose a reason for hiding this comment

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

I think separating out the horizontal and vertical meshes is a sensible choice.


### 2.1 Requirement: OMEGA will use the previously estabilished MPAS Mesh Spec

The OMEGA mesh information should be compatiable with the [MPAS Mesh Specification](https://mpas-dev.github.io/files/documents/MPAS-MeshSpec.pdf).

Choose a reason for hiding this comment

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

Note that the Decomp class ended up creating all the index arrays (CellID, EdgeID, VertexID, XxxOnXxx connectivity arrays). So we can either carry around both class instantiations or we can replicate the pointers to the index arrays here. I guess that's a design decision for sections below, but maybe worth noting here that Decomp has already done the work.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for bringing that up. I forgot to add is how to best interface with the Decomp class. My initial thought was that Decomp class instance could live inside the Mesh class, but there are certainly a variety of options here. Can you point me to your Decomp branch so I can think about that more?

Copy link

Choose a reason for hiding this comment

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

My initial thought was that Decomp class instance could live inside the Mesh class

It seems like Decomp has functionality that intersects with Mesh but doesn't quite belong inside of Mesh.
It should be easy to have a pointer to a Decomp object in Mesh without having that object "live" in Mesh.

Copy link
Author

Choose a reason for hiding this comment

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

@xylar - I tend to think of the decomposition as a property of a given global mesh, but I think what you say makes sense since what we really are creating here is an object for the local mesh information. Perhaps the mesh constructor can take the Decomp object as an argument in order to create the associations connectivity arrays?

components/omega/doc/design/MeshClass.md Outdated Show resolved Hide resolved
Many of the mesh variables are not independent, e.g. areaCell, weightsOnEdge, etc., and can be computed from a reduced set of mesh variables.
This functionality could be used to reduce mesh/restart file size for high resoultion meshes.
Builiding in this flexibility would allow for all mesh-related calculations to be availiable within the code base instead of spreading them amonst various utility programs in MPAS-Tools.
The functions used to compute the dependent mesh information can also be used to verify mesh information provided in the mesh input stream.

Choose a reason for hiding this comment

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

In addition, other useful derived quantities (eg reciprocals) can be helpful to retain for performance, though we can determine the memory/performance tradeoff as we go. Looking at the spec, it seems like other than the index/connectivity arrays, the only thing we really need are xyz coords for each location. It would be nice to minimize the info in the mesh file, but we should get some input from others on the implications for post-processing.

Copy link
Author

Choose a reason for hiding this comment

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

That's a good point about reciprocals etc. I agree that the xyz coords and connectivity information is all we should need (i.e. jigsaw output). It seems like a lot of the mpas_mesh_converter functions could be included here since that's what we use in the meshing workflow to create all the other dependent mesh variables anyway.

Copy link
Author

Choose a reason for hiding this comment

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

I agree that post processing is an important consideration of reducing mesh file info as well.

Copy link

Choose a reason for hiding this comment

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

The main requirement I would suggest adding for post-processing would be a capability (and a standard practice) of writing out all mesh variables to an output file as part of a simulation. Although we could generate such variables on the fly during post processing, it seems desirable to have this done once as part of a model run. Otherwise, the different post-processing runs might construct these variables differently (e.g. on different numbers of cores or with different compilers) and this would potentially complicate reproducibility of the post-processing.

Copy link
Author

Choose a reason for hiding this comment

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

This makes sense and I will include it in the requirements section.


Not all mesh information is required in computing the tendency terms on the device, e.g. lonCell, latCell, etc.
However, other arrays will need to allocated and copied to the device for use in tendency computation.
One option is to template the mesh class so a mesh object can be created for both the host and device, with a method to perform the copy between the two object types.
Copy link

Choose a reason for hiding this comment

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

This should eventually be moved to the algorithm or design section.

Copy link

Choose a reason for hiding this comment

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

I was only referring to the last line starting with "One option..."

Copy link
Author

Choose a reason for hiding this comment

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

I'm removing that line because I believe we've ruled out the template approach in the discussions around this. The Decomp class explicitly creates YAKL arrays for both the host and device for a given variable, so I think it makes sense to use that approach here as well.

brian-oneill pushed a commit to brian-oneill/Omega that referenced this pull request Dec 19, 2023
@sbrus89 sbrus89 force-pushed the sbrus89/omega/design-mesh-class branch from 5f0455e to 89c9174 Compare January 3, 2024 21:55
@philipwjones
Copy link

@sbrus89 Given the update design doc in #48 shall we close this or do you want me to go ahead and merge so we have the history?

@sbrus89
Copy link
Author

sbrus89 commented Jan 25, 2024

@philipwjones - I think merging this would be good, if you're okay with that.

@philipwjones philipwjones merged commit 3f73694 into E3SM-Project:develop Jan 25, 2024
2 checks passed
@mark-petersen mark-petersen removed their request for review January 30, 2024 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants