-
Notifications
You must be signed in to change notification settings - Fork 758
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
Applied user constraints before prolongation in MGTransferPrebuilt #10348
Applied user constraints before prolongation in MGTransferPrebuilt #10348
Conversation
Why have you closed the other PR? |
I basically redid my old PR with this Pull Request. At first I haven't seen the naming convention for the commits, where you need to specify first and last name for each commit. I thought it would be faster for me to just make a clean new PR instead of going back and fixing the author name in all the commits |
I think that now the Travis CI check shouldn't fail |
Nevermind, I have no clue what I am doing wrong and why the Travis CI build is failing... |
Thank you for your first contribution! We require that you set a first name and last name in your git configuration. See |
2160dbc
to
472dc3b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great if you could also add a test for your fix. The easiest way would likely to copy tests/multigrid/transfer_matrix_free_13.cc
and modify it accordingly.
Also, please provide a changelog entry similar to the other ones in doc/news/changes/minor
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... and don't forget a change-log entry!
@tcclevenger can you take a look at this PR? Does this seems reasonable? To make things easier for @mathmerizing , I would propose we merge this PR with a changelog entry and follow up with a test. @tcclevenger can I ask you to do that? |
@tjhei Should I still push the nullptr check proposed by @masterleinad together with a changelog entry? |
Yes, please! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mathmerizing, Thanks for implementing! I've left two comments. I believe the call to distribute constraints should be done before the prolongation not after, and it should not be required in the restriction.
The reasoning is, when doing prolongation to a finer level, there may be non-constrained values which depend on constrained values. Thus, these constrained values need to be correct before the prolongation. During restriction to a coarser level, this won't happen, and the constrained DoFs can be ignored. Also, after both prolongation and restriction, we do not care what values are in constrained DoFs since these will be ignored during smoothing; so no need to distribute constraints after.
It could be that there are two different situations: 1) data after smoothing needs to be corrected (for example with normal flux boundaries) and 2) transfer needs to be corrected. Could that explain the problem? |
@mathmerizing It seems the failing tests happen because no user constraints are stored but they are begin distributed in the transfer. See lines 389-411 in mg_transfer_matrix_free.cc for how we took care of this issue. Something similar could be done here I imagine. |
Is there anything else I should do for this PR? Why is the OSX serial build failing? |
@tcclevenger you communicated about this PR offline, right? Can I ask you to summarize the conclusions? |
I computed the curl boundary constraints via VectorTools::project_boundary_values_curl_conforming_l2 which is currently not setup to work on different level meshes. After a workaround, I now should have the correct constraints on each level. Now it doesn't make a difference, whether the user_constraints are being applied before the prolongation, like @tcclevenger suggested, or after the grid transfer operations. However, in @tcclevenger 's code it became clear that the constraints only need to be applied before prolongation. Thus now this PR should be consistent with MGMatrixFree |
Yes, I tested using this branch on the ASPECT code that prompted the addition of user constraints in MGTransferMatrixFree and it works as expected (same output for prebuilt and matrix-free), so I am satisfied that it is correct. After this pull request is merged I’ll create a separate pull request with a test (same test as for matrix-free transfer). |
awesome, thank you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Thank you.
@masterleinad can you take another look? |
A test is still missing but if @tcclevenger wants to create one in another pull request, thsat is fine with me.
@masterleinad I've created a pull request (#10481) ready for after this is merged. |
Currently only no_normal_flux and zero_boundary_constraints can be used with MGConstrainedDoFs in Multigrid. Since recently, users can also add custom constraints to MGConstrainedDoFs.
In this pull request, I simply apply these user_constraints after the prolongation or restriction in the Multigrid method.
User group post: https://groups.google.com/forum/#!topic/dealii/17V-XvANXGM