-
Notifications
You must be signed in to change notification settings - Fork 71
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
Fixed to the phase-field model #515
base: master
Are you sure you want to change the base?
Conversation
Compute pf_f in the normal direction as soon as possible, because we probably want to use it later in the computations
This way we can both initialise external and internally based on some approximations
<?R | ||
for (ii in 1:cube_faces){ | ||
for (j in 1:length(pressure_boundary_types)) { ?> | ||
CudaDeviceFunction void <?%s paste0(C(my_pressure_boundaries[ii]), pressure_boundary_types[j]) ?>(){ | ||
real_t d = getRho(); |
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.
@shkodm should d be based off a prescribed phase on the boundary rather than from getRho - which I believe calls from PhaseF field
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.
yea, it is then set properly in the if condition below
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 did see this, but figured it was maybe not best to define it with a value knowing it will be overwritten. I was also curious how this worked with the open condition - but perhaps I need to just look at the code a bit deeper. I will try to do this before tomorrow when we meet but have a bit of a marking pile with end of semester.
} | ||
|
||
CudaDeviceFunction real_t getSpecialBoundaryPoint() { | ||
return IsSpecialBoundaryPoint; | ||
} | ||
|
||
/* Calculate the actual wall norm based on the solid | ||
*field */ | ||
CudaDeviceFunction real_t Init_real_wallNorm() |
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 function should be void (shouldn't be returning anything).
<?R | ||
for (ii in 1:cube_faces){ | ||
for (j in 1:length(pressure_boundary_types)) { ?> | ||
CudaDeviceFunction void <?%s paste0(C(my_pressure_boundaries[ii]), pressure_boundary_types[j]) ?>(){ |
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.
remove the "C(", as it doesn't make sense here.
|
||
CudaDeviceFunction void <?R C(my_pressure_boundaries[ii])?>(){ | ||
<?R | ||
for (ii in 1:cube_faces){ |
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 we change this to a pattern of form table with expand.grid, add relevant stuff as columns and use this table everywhere? It is in other places of code and I think is more clear.
BCs = expand.grid(side=1:4, type=c("Pressure","Velocity"), subtype=c("fix_flux","fix_value"))
BCs$side_letter = c("W","E","S","N")[BCs$side]
BCs$name = paste0(BCs$side_letter, BCs$type, "_", BCs$subtype)
and then in Dynamics.R
one can do:
AddNodeType(BCs$name, group="BOUNDARY")
and in Boundary.c.Rt
:
<?R for (bc in rows(BCs)) { ?>
CudaDeviceFunction void <?%s bc$name ?>() {
<?R
...
n=my_normal_directions[bc$side,]
...
?>
}
<?R } ?>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #515 +/- ##
==========================================
- Coverage 44.24% 36.36% -7.89%
==========================================
Files 166 159 -7
Lines 7567 6790 -777
==========================================
- Hits 3348 2469 -879
- Misses 4219 4321 +102
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@nparaskevas you can test it now if you want. I provided the description of different boundary conditions variants in the PR description. Just using Don't forget to add |
Need to normalise by the sum of weights obviously (otherwise would leads to crazy low values of the phase field in random points of the domain)
if ((NodeType & NODE_BOUNDARY) == <?%s paste0("NODE_EPressure", pressure_boundary_types[j]) ?>){ | ||
d = InvasionDrainage == 2? Density_h : Density_l; | ||
myPhaseField = InvasionDrainage == 2? 1.0 : 0.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.
@shkodm I think here you should be setting:
PhaseF = PhaseField; d = getRho();
You then shouldn't need myPhaseField as a variable in this function? I'm not sure what your InvasionDrainage setting here is checking as you specify it should be 1 or -1 in the Dynamics.R. This is causing an issue (i think) for pressure-pressure cases where fpf is used as it effectively forces PhaseField=0 on all pressure boundaries.
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.
@TravisMitchell The documentation in Dynamics.R
is incorrect (copypasted), InvasionDrainage
should have values 1 or 2. d
should be set explicitly because I don't want any streamed values obtained from getRho()
This was the whole point of those if statements, without it would not work properly.
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.
@shkodm, wouldn't it be much simpler/neater to take PhaseField
(zonal setting defined in xml) and then interpolate for d
based off Density_h
and Density_l
. This would mean that the myPhaseField
would be simply set from the zonal condition applied in the xml rather than relying on this InvasionDrainage
setting.
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.
@TravisMitchell yea, interpolation is a good idea. I think I implemented it with an additional setting originally, because InvasionDrainage
(or whatever the name of the flag) is still necessary to know if we want to "reflect/mirror" the boundaries on the inlet (to have proper wall normal not pointing to the other side of domain), or if we want to keep periodic boundaries.
Anyway, I will remove the if statements here later today or tomorrow and double check the boundary conditions.
cc @nparaskevas
Synchronising various fixes I did to the phase-field mode over the past months:
Pressure boundary conditions fixes:
fflux
enforces constant flux -> Breaks when interface enters the outlet - default (i.e withWPressure
orEPRessure
name)EPressurefpf
) makes sure the inlet / outlet phase-field is the same as specified (i.e. sum of distributions forced to be equal to it). Effect: interface propagation stop at the outlet, "impossible-to-displace liquid". But seems to not work with the inletopen
suffix (i.e. EPressureopen) does not enforce the phase-field. Interface propagates into the outlet, splits in 2,there are so strange effects, but otherwise does not look too crazy
InvasionDrainage
is non-zero. And when the boundary conditions are the first / last layers of the domain. Otherwise, one has to compile the model withautosym
option, and mark the inlet / oulets with Symmetry boundaries, so that the periodicity is switched offInvasionDrainage
marks if the case is drainage (non-wetting displacing wetting), or invasion (wetting displacing non-wetting). This sets some densities and phase-field on the inlet / outletMerging with master, because I (and I think other people who are using phase-field model are mostly using master branch and the fixes are quite crucial)
Now doing some testing to make sure I did not mess up when refactoring. So draft PR for now.
cc @TravisMitchell