Implement Darcy Physics - #620
Conversation
|
@ktbolt i think the majority of these file changes have to do with adding any new physics into the current procedural framework. Unfortunate but expected at this point in my opinion. @mmegally I'll do my review in a few hours when I have some time. From my first glance id say that you should remove the sbatch slurm scripts from the commits. These shouldn't be in the testing infrastructure. I'll give a more formal review in a bit though |
|
@ktbolt I went ahead and added a new comment to issue #616 with some details on the file change split, implementation, and XML usage. Let me know if there is anything else I can clarify. To reiterate my comment from there, 31/47 of the file changes are related to the test cases. I will also go ahead and remove the SLURMs from the test folder as zack mentioned right now |
|
@mmegally Got it, thanks for adding the Issue comments ! |
|
@mmegally The Also rename the |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #620 +/- ##
==========================================
- Coverage 73.45% 73.08% -0.37%
==========================================
Files 267 268 +1
Lines 40041 40270 +229
Branches 6706 6738 +32
==========================================
+ Hits 29413 29433 +20
- Misses 10385 10594 +209
Partials 243 243 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
zasexton
left a comment
There was a problem hiding this comment.
Suggested changes for Darcy equation initialization and fiber-flux reconstruction.
…k's suggested changes)
michelebucelli
left a comment
There was a problem hiding this comment.
Thank you @mmegally! I left a few comments.
|
About to push some commits to this fork then we can elevate this draft to a full PR. in the meantime @mmegally can you resolve the active comments that you have addressed along with their appropriate commit tags? you've been doing a great job at fixing up the code so far :) |
zasexton
left a comment
There was a problem hiding this comment.
In my opinion we only piece requiring a bit more review is the zero initialization for the 2D cases to make sure that we are not getting heap error allocations and out-of-bounds. I'll fork this branch and double-check now to make any appropriate changes, if necessary.
michelebucelli
left a comment
There was a problem hiding this comment.
Thanks! I did a second pass, I only have a few minor suggestions left.
…laces and adding parameter dimensions to documentation
Fix Darcy initial pressure indexing
…g exceptions from svmp namespace
zasexton
left a comment
There was a problem hiding this comment.
We are getting very close to closing this PR I believe. I'll make one more fork to address a comment I made about the post.cpp file for higher-order elements. Ultimately I don't think this is a show-stopper for the regular use case of this physics but we should guarantee that it produces expected values if higher-order meshes are ever used with the darcy physics.
Fix higher-order Darcy flux recovery
zasexton
left a comment
There was a problem hiding this comment.
I believe this PR is in a good position to merge
michelebucelli
left a comment
There was a problem hiding this comment.
Thanks for addressing my previous comments! Just a few very minor leftovers, but otherwise approved.
|
Great! Thanks @michelebucelli for the review. @mmegally after making a few minor edits to address the lingering comments I think you should feel free to merge this code :) |
Current situation
See issue #616 on introducing darcy physics.
Release Notes
Closes Add implementation for darcy flow problem #616, Closes Tissue perfusion model #71
Code of Conduct & Contributing Guidelines