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

We now construct a DMLabel for boundary faces. #199

Merged
merged 5 commits into from
Aug 14, 2021

Conversation

jeff-cohere
Copy link
Collaborator

@jeff-cohere jeff-cohere commented Aug 12, 2021

As discussed in #187, this PR introduces the use of DMPlexMarkBoundaryFaces to produce a DMLabel named "boundary" that we can use to enforce boundary conditions. I'm trying to be careful about how we approach this, because our demos are all pretty historical-looking. So this is just another in a series of PRs to improve the organization of the TDycore library.

Additional items:

  • If given -tdy_init_file,TDySetFromOptions allocates storage for the solution vector and friends with TDyCreateVectors and initializes things accordingly. I tried to initialize from a random field, too, but TH does this differently, so this must be done elsewhere for now.
  • TDyCreateVectors and TDyCreateJacobian are documented and can be safely called multiple times (with no effect after the first time).
  • TDyGetBoundaryFaces and TDyRestoreBoundaryFaces are introduced, to provide access to the (undifferentiated) faces on the domain boundary via the label. This provides a simple mechanism for traversing just the boundary faces.
  • The two-point-flux approximation method has been updated to use the "boundary" label. However, I haven't changed the MPFA-O method for marking boundary faces, since it's logically consistent already. I did leave a TODO comment in that section for subsequent cleanup if we want to tighten things up later.

At this point, the set of faces on the domain boundary is generated automatically when the grid is created, so all demos can access it. I think this should close #187. @bishtgautam , let me know if you don't agree. #188 probably demands a little more attention, and the callback/virtual table way of organizing our different algorithms (#197) is looking ever more appealing.

Closes #187

Also, making it safe to call TDyCreateVectors and TDyCreateJacobian more
than once.
Also added TDyGetBoundaryFaces and TDyRestoreBoundaryFaces.
@jeff-cohere jeff-cohere added cleanup Refactor code enhancement New feature or request labels Aug 12, 2021
@codecov-commenter
Copy link

codecov-commenter commented Aug 12, 2021

Codecov Report

Merging #199 (ea0942d) into master (a585b2c) will not change coverage.
The diff coverage is n/a.

❗ Current head ea0942d differs from pull request most recent head 16c8173. Consider uploading reports for the commit 16c8173 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master     #199   +/-   ##
=======================================
  Coverage   67.81%   67.81%           
=======================================
  Files           7        7           
  Lines        1224     1224           
=======================================
  Hits          830      830           
  Misses        394      394           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a585b2c...16c8173. Read the comment docs.

ierr = DMCreateLabel(tdy->dm, "boundary"); CHKERRQ(ierr);
ierr = DMGetLabel(tdy->dm, "boundary", &boundary_label); CHKERRQ(ierr);
ierr = DMPlexMarkBoundaryFaces(tdy->dm, 1, boundary_label); CHKERRQ(ierr);
ierr = DMPlexLabelComplete(tdy->dm, boundary_label); CHKERRQ(ierr);
Copy link

Choose a reason for hiding this comment

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

Note that this Complete() marks the closure of every face, so we might change the comment above

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. Are there cases where this isn't desirable? Just curious (I took this code from an example).

Copy link

Choose a reason for hiding this comment

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

Well, sometimes you want to do a face-specific thing, so you would need to filter out any non-faces, but that is usually easy. FE typically works with the whole boundary whereas FV just wants faces.

Copy link

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

@jeff-cohere jeff-cohere self-assigned this Aug 13, 2021
@bishtgautam
Copy link
Member

@jeff-cohere Can we add a new test or modify the existing test for this new PR?

@jeff-cohere
Copy link
Collaborator Author

@jeff-cohere Can we add a new test or modify the existing test for this new PR?

I'll see if I can modify one of the demos to simplify them a bit, given how much more lifting TDySetFromOptions is doing. This PR might be a little too incremental to show much of a difference there.

@jeff-cohere jeff-cohere merged commit 974f999 into master Aug 14, 2021
@jeff-cohere jeff-cohere deleted the jeff-cohere/boundary-label branch August 14, 2021 23:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup Refactor code enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Read boundary faces from mesh and set boundary condition in a demo
4 participants