Finite Volume PorousFlow

cpgr commented 2 years ago

This is the initial work to support FV in the PorousFlow module.

@loganharbour, would you mind taking a look at this when you get chance before I go too far down the rabbit hole. 😆 I think this was the minimum I could do to get a working FV test problem while having all of the current tests in PorousFlow still working.

moosebuild commented 2 years ago

Job Precheck on 0e2ea04 wanted to post the following:

Your code requires style changes.

A patch was auto generated and copied here
You can directly apply the patch by running, in the top level of your repository:

curl -s https://mooseframework.inl.gov/docs/PRs/21172/clang_format/style.patch | git apply -v

Alternatively, with your repository up to date and in the top level of your repository:

git clang-format ed91e1d359fd3b10c258835ee1d38105a7fc8140

cpgr commented 2 years ago

Hmm, gcc doesn't like this. It looks like I need lots of this->template's in the code.

cpgr commented 2 years ago

Hmm, gcc doesn't like this. It looks like I need lots of this->template's in the code.

Luckily not too many this->templates were required!

moosebuild commented 2 years ago

Job Documentation on 7e7ec29 wanted to post the following:

View the site here

This comment will be updated on new commits.

moosebuild commented 2 years ago

Job Coverage on 7e7ec29 wanted to post the following:

Framework coverage

	b9fd6b	#21172 7e7ec2
	Total	Total	+/-	New
Rate	84.79%	84.77%	-0.01%	5.26%
Hits	83682	83683	+1	1
Misses	15012	15029	+17	18

Diff coverage report

Full coverage report

Modules coverage

Porous flow

	b9fd6b	#21172 7e7ec2
	Total	Total	+/-	New
Rate	96.44%	96.52%	+0.08%	98.40%
Hits	10519	10652	+133	552
Misses	388	384	-4	9

Diff coverage report

Full coverage report

Full coverage reports

Reports

- [`framework`](https://mooseframework.inl.gov/docs/PRs/21172/coverage/framework) - [`chemical_reactions`](https://mooseframework.inl.gov/docs/PRs/21172/coverage/chemical_reactions) - [`combined`](https://mooseframework.inl.gov/docs/PRs/21172/coverage/combined) - [`contact`](https://mooseframework.inl.gov/docs/PRs/21172/coverage/contact) - [`electromagnetics`](https://mooseframework.inl.gov/docs/PRs/21172/coverage/electromagnetics) - [`external_petsc_solver`](https://mooseframework.inl.gov/docs/PRs/21172/coverage/external_petsc_solver) - [`fluid_properties`](https://mooseframework.inl.gov/docs/PRs/21172/coverage/fluid_properties) - [`fsi`](https://mooseframework.inl.gov/docs/PRs/21172/coverage/fsi) - [`functional_expansion_tools`](https://mooseframework.inl.gov/docs/PRs/21172/coverage/functional_expansion_tools) - [`geochemistry`](https://mooseframework.inl.gov/docs/PRs/21172/coverage/geochemistry) - [`heat_conduction`](https://mooseframework.inl.gov/docs/PRs/21172/coverage/heat_conduction) - [`level_set`](https://mooseframework.inl.gov/docs/PRs/21172/coverage/level_set) - [`misc`](https://mooseframework.inl.gov/docs/PRs/21172/coverage/misc) - [`navier_stokes`](https://mooseframework.inl.gov/docs/PRs/21172/coverage/navier_stokes) - [`optimization`](https://mooseframework.inl.gov/docs/PRs/21172/coverage/optimization) - [`peridynamics`](https://mooseframework.inl.gov/docs/PRs/21172/coverage/peridynamics) - [`phase_field`](https://mooseframework.inl.gov/docs/PRs/21172/coverage/phase_field) - [`porous_flow`](https://mooseframework.inl.gov/docs/PRs/21172/coverage/porous_flow) - [`ray_tracing`](https://mooseframework.inl.gov/docs/PRs/21172/coverage/ray_tracing) - [`rdg`](https://mooseframework.inl.gov/docs/PRs/21172/coverage/rdg) - [`reactor`](https://mooseframework.inl.gov/docs/PRs/21172/coverage/reactor) - [`richards`](https://mooseframework.inl.gov/docs/PRs/21172/coverage/richards) - [`scalar_transport`](https://mooseframework.inl.gov/docs/PRs/21172/coverage/scalar_transport) - [`solid_properties`](https://mooseframework.inl.gov/docs/PRs/21172/coverage/solid_properties) - [`stochastic_tools`](https://mooseframework.inl.gov/docs/PRs/21172/coverage/stochastic_tools) - [`tensor_mechanics`](https://mooseframework.inl.gov/docs/PRs/21172/coverage/tensor_mechanics) - [`thermal_hydraulics`](https://mooseframework.inl.gov/docs/PRs/21172/coverage/thermal_hydraulics) - [`xfem`](https://mooseframework.inl.gov/docs/PRs/21172/coverage/xfem)

Warnings

framework new line coverage rate 5.26% is less than the suggested 90.0%

This comment will be updated on new commits.

mangerij commented 2 years ago

Hi, I noticed Ferret failed here but when I looked at CIVET, I saw that all tests passed. Anyone know why?

loganharbour commented 2 years ago

Hi, I noticed Ferret failed here but when I looked at CIVET, I saw that all tests passed. Anyone know why?

Unrelated issue. Should be good now.

@loganharbour, would you mind taking a look at this when you get chance before I go too far down the rabbit hole.

👍🏼 on my list.

moosebuild commented 1 year ago

Job Precheck on eeb4cdd wanted to post the following:

A change of the following file(s) triggered this check:

libmesh

The following file(s) are unchanged:

conda/mpich/conda_build_config.yaml conda/libmesh/meta.yaml

The libmesh submodule or configuration was changed but the conda build config was not

cpgr commented 1 year ago

I've just realised that it doesn't seem like there is any way to find the documentation (https://mooseframework.inl.gov/docs/PRs/21172/site/automatic_differentiation/templated_objects.html) I wrote. It follows on from https://mooseframework.inl.gov/docs/PRs/21172/site/automatic_differentiation/ but I can't find out where this links from. It should possibly be listed in 'Application development'?

GiudGiud commented 1 year ago

@cpgr the AD page is linked to from some of the systems using AD https://mooseframework.inl.gov/syntax/Kernels/ or https://mooseframework.inl.gov/syntax/Materials/ for example

cpgr commented 1 year ago

@cpgr the AD page is linked to from some of the systems using AD https://mooseframework.inl.gov/syntax/Kernels/ or https://mooseframework.inl.gov/syntax/Materials/ for example

Thanks! I was wondering how anyone would find this.

lindsayad commented 1 year ago

@cpgr I was wondering if you looked at our porous Navier-Stokes slides at https://mooseframework.inl.gov/slides/index.html#/ whether you could tell me what the difference between that and the equations solved by the porous_flow module is? I'm hoping there is a difference haha, and if so we should think about documenting that somewhere so our users know where to go for what application/modeling-assumptions is relevant to them.

cpgr commented 1 year ago

@cpgr I was wondering if you looked at our porous Navier-Stokes slides at https://mooseframework.inl.gov/slides/index.html#/ whether you could tell me what the difference between that and the equations solved by the porous_flow module is? I'm hoping there is a difference haha, and if so we should think about documenting that somewhere so our users know where to go for what application/modeling-assumptions is relevant to them.

The governing equations for the porous_flow module are outlined here -https://mooseframework.inl.gov/modules/porous_flow/governing_equations.html

They are basically conservation of mass of a fluid component + Darcy's law.

I think the main difference would be that this module is set up for more classical reservoir engineering cases, and can model an arbitrary number of fluid phases (most common use case would be 2 I imagine - a gas phase and a liquid phase), but each phase can have an arbitrary number of fluid components (eg, the liquid can be water + salt + dissolved gas component etc).

The FV stuff here (which is basically an FV equivalent to the FE objects already in this module) is to make it much easier to model cases where there are discontinuities in one of the variables, such as gas saturation , due to the reservoir properties.

lindsayad commented 1 year ago

It's fun learning about this. A really dumb question that I haven't answered for myself yet: if you were to model a single phase single component problem, what would your nonlinear variable(s) be? Is it just the pressure?

cpgr commented 1 year ago

Yes

cpgr commented 1 year ago

I've been using this for a little while now and it all seems to be working.

The only part I'm not certain on is in Coupleable.C. I needed to template coupledDofValues to template all the porous flow materials, so added

template <>
const GenericVariableValue<false> &
Coupleable::coupledGenericDofValue<false>(const std::string & var_name, unsigned int comp) const
{
  return coupledDofValues(var_name, comp);
}

template <>
const GenericVariableValue<true> &
Coupleable::coupledGenericDofValue<true>(const std::string & var_name, unsigned int comp) const
{
  return adCoupledValue(var_name, comp);
}

but I'm not sure on the AD one.

lindsayad commented 1 year ago

Yea that's probably not quite right. The dof values should return all the degree of freedom values in the solution vector associated with a given element, whereas the coupled value api's return the finite element solution evaluated at the element quadrature points

cpgr commented 1 year ago

I couldn't find an adCoupledDofValues method - before I try and make one can I check that I'm not missing anything?

lindsayad commented 1 year ago

you're not missing anything 😄

GiudGiud commented 1 year ago

@cpgr are you waiting for a review on this?

cpgr commented 1 year ago

@cpgr are you waiting for a review on this?

Yeah, but no rush. This was to check that what I have done so far is ok. I know that there is a lot of work to make it work with all of the exiting PorousFlow objects, I just don't have time until next month to keep going.

If you or @loganharbour could just take a quick look and let me know if there is anything that I am not doing right that would be great.

GiudGiud commented 1 year ago

Sounds good. I ll add it to my list and I think @lindsayad will want to have a look as well since this is FV

lindsayad commented 1 year ago

Ah I had no idea you were waiting on a review. Sorry @cpgr !

lindsayad commented 1 year ago

I should have taken the fact that you marked it as ready for review as a cue haha

cpgr commented 1 year ago

No worries guys!

cpgr commented 1 year ago

Something has obviously changed - the flux kernel I was using no longer works! Any hints you could give me?

lindsayad commented 1 year ago

Hmm I think with @grmnptr's recent changes to remove ghost cells this might have plunged your neighbor material property evaluations when you're on a boundary into oblivion. What I would suggest, if you're wiling to wait, is to let #22130 go in, and then I can come push a commit to your PR updating your flux kernel. Would that be ok? I know you've already waited a long time, so if you're eager to have this in, then I can probably come up with a patch now

cpgr commented 1 year ago

Waiting is fine!

lindsayad commented 1 year ago

Alrighty, I'll re-save this and come back once #22130 is merged

lindsayad commented 1 year ago

Ok #22130 is in. Want the follow-on #23233 to get through and with those two I have a pretty good idea about where I want to take this. I don't think it'll be a massive re-write like I initially thought

cpgr commented 1 year ago

Let me know if there is anything I can do!

lindsayad commented 1 year ago

You've done plenty @cpgr. It's up to us to make sure your hard work doesn't go to waste

lindsayad commented 1 year ago

Ok @cpgr I finally got around to it! Sorry it took forever! I rebased onto a more recent MOOSE commit and then added one of my own. The important change is that we've removed ghost cells; so you have to be careful to avoid indexing into data that logically corresponds to a ghost cell, e.g. a "neighbor" when you are on a boundary face. In order to circumvent this change, I added a new FVFluxBC class that mimics the FVFluxKernel class you created but makes sure to avoid that out-of-bounds indexing.

I tried to read about the porous flow module a bit, but I think it would be best if you looked at that new BC class and decided whether there might be a way to generalize it to more phases like you have for the flux kernel. Anyway hopefully this is helpful and you can develop on top of this and we can get this merged soon!

cpgr commented 1 year ago

Thanks so much @lindsayad. I'll take a look over the next few days

cpgr commented 1 year ago

I'm trying to understand why making a new flux bc fixes this? Does a FVFluxKernel not evaluate on the boundary faces anymore, meaning that a flux in/out must be added using a BC?

lindsayad commented 1 year ago

If a FVFluxBC object exists on a boundary, then the FVFluxKernel does not execute on that boundary. Here is the logic

bool
FVFluxKernel::skipForBoundary(const FaceInfo & fi) const
{
  // Boundaries to avoid come first, since they are always obeyed
  if (avoidBoundary(fi))
    return true;

  // We're not on a boundary, so in practice we're not 'skipping'
  if (!onBoundary(fi))
    return false;

  // Blanket forcing on boundary
  if (_force_boundary_execution)
    return false;

  // Selected boundaries to force
  for (const auto bnd_to_force : _boundaries_to_force)
    if (fi.boundaryIDs().count(bnd_to_force))
      return false;

  // If we have flux bcs then we do skip
  const auto & flux_pr = _var.getFluxBCs(fi);
  if (flux_pr.first)
    return true;

  // If we don't have flux bcs *and* we do have dirichlet bcs then we don't skip. If we don't have
  // either then we assume natural boundary condition and we should skip
  return !_var.getDirichletBC(fi).first;
}

cpgr commented 1 year ago

Does the FVBC you made need to add a Dirichlet BC as well? In my admittedly limited understanding of how the FV system works won't specifying the flux naturally lead to that boundary pressure? I did the scientific thing and commented out the bit that adds a Dirichlet BC and the results appeared identical hahaha.

Edit: this bit of code

auto diri_params = FVDirichletBC::validParams();
diri_params.applySpecificParameters(_pars, {"variable", "boundary"});
diri_params.addPrivateParam("_moose_app", &_app);
diri_params.set<Real>("value") = _pp_value; 
_fv_problem.addFVBC("FVDirichletBC", name() + "_diri", diri_params);
 _fv_problem.fvBCsIntegrityCheck(false);

lindsayad commented 1 year ago

One would hope. The reason I added it is for cell gradients. Cell gradients use Green-Gauss for their calculation so they need face values. If a Dirichlet BC exists then it will use that as the face value; if not, then by default we will use a two term extrapolation to determine the boundary face value based on the cell gradient (so this is actually an implicit calculation) and the cell center value. So practically speaking this extrapolated value probably won't match the Dirichlet value exactly, but hopefully it's close and is only an error of O(h^2).

You are certainly welcome to remove that code if you are happy with the results as they are

cpgr commented 1 year ago

oops - accidentally closed this!

cpgr commented 1 year ago

The missing markdown documentation I can fix. The failing Modules parallel test throws an error when it runs the --n-threads=2 tests

A 'FVBoundaryCondition' object already exists with the name 'left_diri'.

Does this just need an

if (_tid == 0)
 add Dirichlet BC here

lindsayad commented 1 year ago

Yes; that was a stupid oversight by me.

cpgr commented 1 year ago

I'd still like to see a Jacobian test, but otherwise this is good to go as far as I'm concerned.

I just added a Jacobian test of the FV kernels

cpgr commented 1 year ago

Above and beyond on that last commit @cpgr 😄

sed and some search and replace!

GiudGiud commented 1 year ago

@lindsayad do you need an external review on your commit or are we below the "significance" threshold?

lindsayad commented 1 year ago

I don't think it's very significant, so I don't think it needs it. Feel free to review if you wish obviously, but I think we're good to move forward without more review

GiudGiud commented 1 year ago

@cpgr please let me know when you want this merged

lindsayad commented 1 year ago

I bet he's ready to have it merged now 😄

cpgr commented 1 year ago

Haha - what's a few extra days!

cpgr commented 1 year ago

Thanks @lindsayad for your help getting this through!

lindsayad commented 1 year ago

You're welcome! Thanks for your patience in the process!

idaholab / moose