Fix slip BCs for IBM #789
Fix slip BCs for IBM #789
Conversation
sbryngelson
added
bug
|
@sbryngelson I will look at this today. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #789 +/- ##
==========================================
- Coverage 43.76% 43.73% -0.03%
==========================================
Files 65 65
Lines 19047 19042 -5
Branches 2324 2324
==========================================
- Hits 8335 8328 -7
- Misses 9305 9307 +2
Partials 1407 1407 ☔ View full report in Codecov by Sentry. |
|
@sbryngelson I just left one review comment. Other than that it looks fine and makes sense to me. And it's true that we need some slip IBM test cases. I can either create another PR or add them to the test suite if you give me the permission to push to this branch. |
|
@haochey, I don't see any review comments. You probably didn't "post" them. Can you add them to the test suite after I merge them? I will wait until PACE Phoenix is back up and running so CI all passes, probably tomorrow or Friday. |
| @@ -209,7 +207,9 @@ contains | |||
|
|
|||
| ! Calculate velocity of ghost cell | |||
| if (gp%slip) then | |||
| norm = levelset_norm%sf(j, k, l, patch_id, :) | |||
| norm = levelset_norm%sf(gp%loc(1), gp%loc(2), gp%loc(3), gp%ib_patch_id, :) | |||
| buf = sqrt(sum(norm**2)) | |||
There was a problem hiding this comment.
I think these lines are redundant because levelset_norm%sf(gp%loc(1), gp%loc(2), gp%loc(3), gp%ib_patch_id, :) is already a norm vector.
And j = gp%loc(1), k = gp%loc(2) , l = gp%loc(3) , so norm = levelset_norm%sf(j, k, l, patch_id, :) should be fine.
There was a problem hiding this comment.
I think I agree, but what about the other part (line 211 and 222 that have buf = ... and norm = norm/buf? that seems different/new
There was a problem hiding this comment.
These two lines seem to normalize the norm to be unit vectors while norm are already unit vectors because levelset_norms' are unit vectors.
There was a problem hiding this comment.
i suspect this was not added by @henryleberre for no reason, is there any way to ensure levelset_norm subroutines always normalize without just doing it here?
There was a problem hiding this comment.
Make sense to me! It will rely on levelset subroutines to normalize them before output them. I think this is the reason why Henry added these two lines.
There was a problem hiding this comment.
Btw there's a corner case that needs to be taken care of, when norm = 0
There was a problem hiding this comment.
what do you suggest doing in this case, @anandrdbz? You can write code to my branch/PR here.
There was a problem hiding this comment.
Okay I'll take a look, do I have push permissions ?
Yes, sounds good. |
This small PR grabs fixes from @henryleberre's PR #737 for IBMs with slip-boundary cases. It apparently came from a problem that @anshgupta1234 found.
@haochey, can you have a look at the code and let me know if this makes sense or where it comes from?
Also, it appears that we don't have any slip IBM cases in the test suite. Is this true? If so, we need to add them.