Skip to content
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

Draft
wants to merge 23 commits into
base: master
Choose a base branch
from
Draft

Conversation

shkodm
Copy link
Member

@shkodm shkodm commented May 27, 2024

Synchronising various fixes I did to the phase-field mode over the past months:

  • Split initialisation of wall normals in 2 parts, so it is possible to supply user defined wall normals using RunR
  • Fix various edge cases and bugs with wetting boundary conditions
  • Fix pressure boundary conditions
  • Add markers for the saturation

Pressure boundary conditions fixes:

  • Now use proper ZouHe boundary conditions, with 3 variants. (Some can be removed later)
    • fflux enforces constant flux -> Breaks when interface enters the outlet - default (i.e with WPressure or EPRessure name)
    • fixed phase-field (e.g. 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 inlet
    • open 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
  • Gradient / Laplacian on the velocity / pressure boundaries is always zero (does not touch the other side of the domain)
  • Wall normal calculations do not take into account the other side of the domain (periodicity is switched off). This works only for the case of X direction and when the flag InvasionDrainage is non-zero. And when the boundary conditions are the first / last layers of the domain. Otherwise, one has to compile the model with autosym option, and mark the inlet / oulets with Symmetry boundaries, so that the periodicity is switched off
  • InvasionDrainage 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 / outlet
  • Apply boundary conditions before VTK export, so the inlet / outlet zones don't look strange due to streaming

Merging 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

<?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();
Copy link
Member

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

Copy link
Member Author

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

Copy link
Member

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()
Copy link
Member

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]) ?>(){
Copy link
Member

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){
Copy link
Member

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 } ?>

Copy link

codecov bot commented May 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 36.36%. Comparing base (2362a9d) to head (98accf4).
Report is 24 commits behind head on master.

Current head 98accf4 differs from pull request most recent head 9a0f8d6

Please upload reports for the commit 9a0f8d6 to get more accurate results.

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     
Flag Coverage Δ
d2q9 ?
d3q27_PSM_NEBB 36.36% <ø> (+0.01%) ⬆️
d3q27_pf_velocity ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@shkodm
Copy link
Member Author

shkodm commented Jun 4, 2024

@nparaskevas you can test it now if you want. I provided the description of different boundary conditions variants in the PR description. Just using WPressure on the inlet and EPressurefflux or EPressurefpf or EPressureopen on the outlet would work, but you should play around.

Don't forget to add InvasionDrainage value="2" to the setting in case of drainage, or value = 1 in case of invasion

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;
}

Copy link
Member

@TravisMitchell TravisMitchell Sep 5, 2024

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.

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

@shkodm shkodm Sep 5, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants