-
Notifications
You must be signed in to change notification settings - Fork 89
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: try this density treatment for gravity #3337
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3337 +/- ##
===========================================
- Coverage 57.56% 57.56% -0.01%
===========================================
Files 1092 1092
Lines 97800 97837 +37
===========================================
+ Hits 56302 56320 +18
- Misses 41498 41517 +19 ☔ View full report in Codecov by Sentry. |
} | ||
denom++; | ||
} | ||
if( denom > 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.
Could a phase potentially be absent in both cells? Or is this checked elsewhere?
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 can be absent in both yes, there is a check below but it would be better to check even outside of the function
Just don't want to add more complexity for this PR
for( localIndex i = 0; i < numFluxSupportPoints; ++i ) | ||
{ | ||
localIndex const er = seri[i]; | ||
localIndex const esr = sesri[i]; | ||
localIndex const ei = sei[i]; | ||
|
||
bool const phaseExists = (phaseVolFrac[er][esr][ei][ip] > 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.
In some cases I have seen saturation weighted density in this case. Not sure what the benefit of that is over a simpler mean. Also looks like it might complicate the derivatives.
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.
Yeah, it should be more accurate, but I don't want to complicate the derivatives
review please? |
@CusiniM, @castelletto1, @cssherman, @jhuang2601, @rrsettgast, and/or @wrtobin could you please review? |
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 feel it should be weighted with the saturation but this is better than before.
or maybe using the mass fraction since it's a density. |
need to adjust thermal part too |
currently density in gravity term is
0.5(rho_i+rho_j)
wherei
andj
are the cellshere we are changing it to more commonly used treatment that checks phase presence leading to:
0.5(rho_i+rho_j)
when phase is present in both cellsrho_i
when only ini
rho_j
when only inj
probably not a big deal but better to be consistent with mainstream way