-
Notifications
You must be signed in to change notification settings - Fork 67
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
Fix Flows/Flores for NNCs #874
Conversation
jenkins build this 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.
Looks mostly fine, a few minor issues to address before merging.
@@ -565,13 +565,13 @@ private: | |||
} | |||
|
|||
if (anyFlows) { | |||
flowsInfo_.reserve(numCells, 6 * numCells); | |||
flowsInfo_.reserve(numCells, 6); |
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.
This looks strange to me, expecting 6 flow numbers regardless of number of cells?
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.
Thanks. I misread the SparseTable class. Will revert.
@@ -661,6 +661,7 @@ public: | |||
ADVectorBlock darcyFlux(0.0); | |||
const IntensiveQuantities& intQuantsIn = model_().intensiveQuantities(globI, /*timeIdx*/ 0); | |||
// Flux term. | |||
short loc = 0; |
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.
Place the variable after the block open bracket on line 665 to ensure its scope is as limited as possible.
for (unsigned phaseIdx = 0; phaseIdx < numEq; ++ phaseIdx) { | ||
flowsInfo_[globI][bdyInfo.dir].flow[phaseIdx] = adres[phaseIdx].value(); | ||
for (unsigned eqIdx = 0; eqIdx < numEq; ++ eqIdx) { | ||
flowsInfo_[globI][nbInfos.size() + bfIndex].flow[eqIdx] = adres[eqIdx].value(); |
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 check I understood this correctly: this assumes that for every cell, flowsInfo_ stores a number of objects equal to the number of neighbours plus the number of boundaries, and the flows for boundaries come last.
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.
Yes. That is correct. That is also why I only resize to (numCells, 6) i.e guessing on 6 neighbors. (your comment above)
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 is a reserve not a resize, but apart from that I still do not understand. A typical grid with ~1000 cells will have ~6000 neighbours, so reserving just space for 6 in the data array will ensure that the data array gets reallocated many times. How does this relate to the boundaries being added as well? If anything, that would increase the required space.
jenkins build this please |
jenkins build this please |
Restores NNC output for Flows.
See #873