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

Add local face fields #868

Merged
merged 47 commits into from
May 31, 2023
Merged

Add local face fields #868

merged 47 commits into from
May 31, 2023

Conversation

lroberts36
Copy link
Collaborator

@lroberts36 lroberts36 commented Apr 28, 2023

PR Summary

This PR starts to implement face, edge, and nodal variables. Specifically, it makes the changes required within the Variable and Metadata classes to allow Variables to store any type of field and allows these types of variables to be included in packs (both legacy and sparse packs). It does not deal with any non-block local operations on these types of variables (e.g. boundary communication, prolongation, restriction), mainly to keep the PR small. This borrows heavily from #764 and is built on #867. Please review #867 first if you haven't already.

Todo:

  • Include field type dependent shapes in Metadata and allocate Variable data and coarse data based on these shapes.
  • Move all ParArray6Ds to ParArrayNDs so we can add an extra index for indexing the face.
  • Allow for non-cell centered variables in packs. There may need to be a discussion of how we want this interface to look
  • Add test to ensure new variables types are behaving as expected

PR Checklist

  • Code passes cpplint
  • New features are documented.
  • Adds a test for any bugs fixed. Adds tests for new features.
  • Code is formatted
  • Changes are summarized in CHANGELOG.md
  • CI has been triggered on Darwin for performance regression tests.
  • Docs build
  • (@lanl.gov employees) Update copyright on changed files

@lroberts36 lroberts36 mentioned this pull request May 1, 2023
13 tasks
@lroberts36 lroberts36 changed the title WIP: Add local face fields Add local face fields May 2, 2023
@lroberts36 lroberts36 added the enhancement New feature or request label May 2, 2023
Copy link
Collaborator

@Yurlungur Yurlungur 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 cool! A few questions/nitpicks below.

src/bvals/comms/bnd_info.hpp Show resolved Hide resolved
src/interface/sparse_pack.hpp Outdated Show resolved Hide resolved
src/interface/variable.hpp Outdated Show resolved Hide resolved
src/interface/variable_pack.hpp Show resolved Hide resolved
src/basic_types.hpp Outdated Show resolved Hide resolved
src/interface/variable_pack.hpp Outdated Show resolved Hide resolved
src/interface/variable_state.hpp Outdated Show resolved Hide resolved
src/utils/multi_pointer.hpp Show resolved Hide resolved
tst/unit/test_mesh_data.cpp Show resolved Hide resolved
tst/unit/test_mesh_data.cpp Outdated Show resolved Hide resolved
@forrestglines
Copy link
Collaborator

@lroberts36 Just to let you know, I'm getting through a review of this PR, best bet I'll be able to finish it tomorrow

Copy link
Collaborator

@forrestglines forrestglines left a comment

Choose a reason for hiding this comment

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

Looks good so far. I have a few questions relating to feature directions:

  1. Right now the different faces, edges, are packaged into one array, which means some wasted space. Does the outward facing interface have room to separate out those arrays in case we want to do internal code surgery to save that space?

  2. I can see a few useful operations that might go into Parthenon to help with topological elements -- interpolating from faces to cells and vice versa, Curls on faces to cell centers, etc. I think we should get those into Parthenon rather than downstream codes.

src/basic_types.hpp Show resolved Hide resolved
src/interface/variable_pack.hpp Outdated Show resolved Hide resolved
src/interface/variable_pack.hpp Outdated Show resolved Hide resolved
src/interface/variable_pack.hpp Show resolved Hide resolved
src/interface/variable_pack.hpp Show resolved Hide resolved
@Yurlungur
Copy link
Collaborator

WIth one approval from LANL and one from AthenaPK, are we good to merge this?

@lroberts36
Copy link
Collaborator Author

WIth one approval from LANL and one from AthenaPK, are we good to merge this?

I think so, but let's wait til the next Parthenon meeting.

@lroberts36
Copy link
Collaborator Author

@forrestglines:

1. Right now the different faces, edges, are packaged into one array, which means _some_ wasted space. Does the outward facing interface have room to separate out those arrays in case we want to do internal code surgery to save that space?

I think it shouldn't be too hard to switch to having no wasted space, but at the expense of three times as many allocations.

2. I can see a few useful operations that might go into Parthenon to help with topological elements -- interpolating from faces to cells and vice versa, Curls on faces to cell centers, etc. I think we should get those into Parthenon rather than downstream codes.

I think things like curls, etc. should maybe go into update.hpp but we can save that for a future PR. I have to think a little more about interpolation from one kind of topological element to another, but I would also say let's save that for a future PR.

Copy link
Collaborator

@bprather bprather left a comment

Choose a reason for hiding this comment

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

As I said I've been using this for a while without problems -- or at least, without problems caused by this code.

Debug builds were my friend when moving to face-centered variables, even for non-segfault issues. I like them and I think we should push them at users more aggressively as the solution to a broad range of bugs, not just segfaults and bounds errors.

src/basic_types.hpp Outdated Show resolved Hide resolved
@lroberts36 lroberts36 changed the base branch from lroberts36/rename-cell-variable to develop May 31, 2023 19:49
@lroberts36 lroberts36 enabled auto-merge (squash) May 31, 2023 22:25
@lroberts36 lroberts36 merged commit 099c883 into develop May 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants