CFD-GO / TCLB

TCLB - Templated MPI+CUDA/CPU Lattice Boltzmann code
https://tclb.io
GNU General Public License v3.0
180 stars 71 forks source link

Add wetting boundary conditions to the phase field model #431

Closed shkodm closed 1 year ago

shkodm commented 1 year ago

I also splitted Dynamics.c.Rt in two files with all wetting boundary conditions code in Boundary.c.Rt to make it more readable. The file is included and preprocessed using Rtemplate <?RT ?> syntax at the bottom of Dynamics.c.Rt.

The code in Boundary.c.Rt should be ready to review already, @TravisMitchell, would be useful if you go through. It should be quite documented. I need to go through the code in Dynamics.c.Rt and Dynamics.R again to make sure I did not remove / added something by accident when cleaning up code and fixing merge conflicts. I also want to double check the performance again just in case. This is why for now PR is DRAFT, once I verify this I will convert it into a normal PR.

codecov-commenter commented 1 year ago

Codecov Report

Patch and project coverage have no change.

Comparison is base (a74ad83) 32.52% compared to head (1cc1ec6) 32.52%.

:mega: This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files ```diff @@ Coverage Diff @@ ## develop #431 +/- ## ======================================== Coverage 32.52% 32.52% ======================================== Files 152 152 Lines 6453 6453 ======================================== Hits 2099 2099 Misses 4354 4354 ``` | Flag | Coverage Δ | | |---|---|---| | d2q9 | `30.45% <ø> (ø)` | | | d2q9_bc | `26.73% <ø> (ø)` | | | d2q9_kuper | `27.58% <ø> (ø)` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CFD-GO#carryforward-flags-in-the-pull-request-comment) to find out more. | [Impacted Files](https://codecov.io/gh/CFD-GO/TCLB/pull/431?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CFD-GO) | Coverage Δ | | |---|---|---| | [src/Global.h.Rt](https://codecov.io/gh/CFD-GO/TCLB/pull/431?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CFD-GO#diff-c3JjL0dsb2JhbC5oLlJ0) | `50.00% <ø> (ø)` | | | [src/Handlers/cbRunR.cpp.Rt](https://codecov.io/gh/CFD-GO/TCLB/pull/431?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CFD-GO#diff-c3JjL0hhbmRsZXJzL2NiUnVuUi5jcHAuUnQ=) | `66.66% <ø> (ø)` | | | [src/LatticeAccess.inc.cpp.Rt](https://codecov.io/gh/CFD-GO/TCLB/pull/431?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CFD-GO#diff-c3JjL0xhdHRpY2VBY2Nlc3MuaW5jLmNwcC5SdA==) | `60.86% <ø> (ø)` | | | [src/LatticeContainer.h.Rt](https://codecov.io/gh/CFD-GO/TCLB/pull/431?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CFD-GO#diff-c3JjL0xhdHRpY2VDb250YWluZXIuaC5SdA==) | `83.33% <ø> (ø)` | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CFD-GO). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=CFD-GO)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

llaniewski commented 1 year ago

I think the optimise_for_static_access is not working right for the 2D models. the "big" flag was triggered for stencils above 27, which never happened for 2D models, and thus, this code was never generated for 2D models. Now that you generate it, it fails on compilation. I think this comes from an error (wrong assumption) in conf.R. Try replacing (https://github.com/CFD-GO/TCLB/blob/develop/src/conf.R#L777):

tab3 = rep(TRUE,9)

with one which will take into account what are the maximal stencils in each direction:

tab3 = c(mins<0,TRUE,TRUE,TRUE,maxs>0)
shkodm commented 1 year ago

I think the optimise_for_static_access is not working right for the 2D models. the "big" flag was triggered for stencils above 27, which never happened for 2D models, and thus, this code was never generated for 2D models. Now that you generate it, it fails on compilation.

I will fix the issue with 2D models, but actually big flag was not supposed to be triggered for 2D model. Big means no optimisation (so corresponds to optimise_static_access = FALSE). I had it the other way around, that is why new 2D models build was triggered and was failing.

shkodm commented 1 year ago

@TravisMitchell you can review now. I already tested the performance (everything is still god), will test again if the results are the same as I had before.

@llaniewski I will switch the path inside <?RT > tag once rtemplate PR is merged

shkodm commented 1 year ago

@TravisMitchell would you have time to review this one soon? I validated it on capillary intrusion already, the droplet spread jobs are currently stuck in queue.

TravisMitchell commented 1 year ago

@shkodm I will do today

shkodm commented 1 year ago

@TravisMitchell I tested all methods on the Washburn Law again after your refactoring, it still works. I think you can approve this PR.

@llaniewski I think it can be ready to merge. But please squash my commits when merging (there should be an option), there is no need for my 100 commits to be in the git history of the main repo.

Also I think to keep:

<?RT models/multiphase/d3q27_pf_velocity/Boundary.c.Rt ?> 

as it is (not using just Boundary.c.Rt), because this way we don't need to update rtemplate package everywhere.