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.
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
Enforcing inflow-outflow boundary solvability #129
Enforcing inflow-outflow boundary solvability #129
Changes from all commits
e97b7fa
a0a1b7a
5107115
4d5c939
5e83b8d
0d64ad3
c43f075
a672107
b8ea32b
3115f03
9cb268a
741c157
1f2c5f5
e2fec50
c4bd659
9ce8942
e057065
6d567d5
1a529b8
54ee91e
3bb2f8d
71ba07b
b5c73ad
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
When I first saw "direction_dependent", it wasn't clear to me what this meant. The rest of the BCTypes are mathematical BCs. Why is "direction_dependent" the mathematical BC here instead of "mixed" or "Robin"?
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.
We could remove it from amrex::BCType and use
user_*
.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.
mixed
might be okay too. But Whatmixed
means is likely user dependent, whereas other types (e.g., reflect_odd, foextrap, etc.) are clear on what they mean and AMReX can fill them for the user.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.
We added this math BC type in AMReX (AMReX-Codes/amrex#3965) where the cells are to be treated differently based on in/out flow. Perhaps @asalmgren could comment on the naming.
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.
To amrex, that's a user type that it does not know how to handle.
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.
On the user side, the user could define
auto my_preferred_name = amrex::BCType::user_1;
.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.
I think the reason for adding a special math type rather than just using user_1 was so that it can be used consistently across dependent codes, say, amr-wind and amrex-hydro, and so it does not conflict with other custom types that may be added in the future.
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.
amr-wind uses amrex-hydro. So it could use the types defined in amrex-hydro. (If it does not use amrex-hydro, then it probably does not matter.)
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.
If this is meant to be used with both the Nodal and MAC projections, then since the BC types for those linops differ and I don't think that can be used for this purpose.
If this could be used more generally, in additional application codes, then maybe instead of having the BCRec it could be something more like a flag that indicates whether to include the face in the sum or not?