Merged
Conversation
the exception is GetCornerPointsAllFaces, which actually changes the state
now CPrimalGridFEM::nFaces is initialized already in the constructor
pcarruscag
reviewed
Aug 16, 2021
Update Common/src/geometry/primal_grid/CPrimalGridFEM.cpp Co-authored-by: Pedro Gomes <38071223+pcarruscag@users.noreply.github.com>
Update Common/include/geometry/primal_grid/CPrimalGridFEM.hpp Co-authored-by: Pedro Gomes <38071223+pcarruscag@users.noreply.github.com>
5 tasks
pcarruscag
approved these changes
Aug 17, 2021
Co-authored-by: TobiKattmann <31306376+TobiKattmann@users.noreply.github.com>
this regression was only detected by the testcase discadj_topol_optim in parallel_regression_AD.py!
Co-authored-by: Pedro Gomes <38071223+pcarruscag@users.noreply.github.com>
maxaehle
commented
Aug 22, 2021
Co-authored-by: Pedro Gomes <38071223+pcarruscag@users.noreply.github.com>
…into fix_primalgrid_const
added 5 commits
August 25, 2021 18:09
Only one of them is used for each instance. This is currently checked, I'll remove checks later.
pcarruscag
reviewed
Aug 25, 2021
added 3 commits
August 25, 2021 19:25
The bool CPrimalGrid::gi indicated whether CPrimalGrid::GlobalIndex_DomainElement held a GlobalIndex or a DomainElement. We used it to check whether there each instance uses only one of them, the regression tests succeeded.
Contributor
TobiKattmann
left a comment
There was a problem hiding this comment.
Well that is quite a bit addition to this PR :) Thanks for the cleanup @maxaehle and @pcarruscag
I really appreciate the sometimes detailed commit messages and some good quality doxygen documentation (I already noticed that on the NdFlattener 👍 ). Keep that going and I hope this spreads a bit (me included ;) )
pcarruscag
reviewed
Aug 27, 2021
pcarruscag
reviewed
Aug 27, 2021
Comment on lines
-38
to
-42
| unsigned short CHexahedron::nFaces = 6; | ||
|
|
||
| unsigned short CHexahedron::nNodes = 8; | ||
|
|
||
| unsigned short CHexahedron::nNeighbor_Elements = 6; |
Member
There was a problem hiding this comment.
@TobiKattmann, reply to your question about faces and neighbors.
But the index of the neighbor can be -1 to indicate it a) does not exist; b) does not exist in this rank.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed Changes
CPrimalGrid::Get...as constCPrimalGridFEM::GetCornerPointsAllFacesdid set the membernFaces. To make it const, I introducedCPrimalGridFEM::elementtype_to_nFacesand initializednFacesin the constructor.const CPrimalGrid* eleminCInterpolator::ReconstructBoundary(Fix the neighbor-finding inCInterpolator::ReconstructBoundary#1346).PR Checklist
Put an X by all that apply. You can fill this out after submitting the PR. If you have any questions, don't hesitate to ask! We want to help. These are a guide for you to know what the reviewers will be looking for in your contribution.