Extend VoronoiFVM to incorporate FEM-computed velocity fields provided by ExtendableFEM

[Da-Be-Ru]

We (@pjaap, @chmerdon and me) would like for VoronoiFVM.jl to admit FEM solutions as velocity fields in solving convection-diffusion systems.
For that purpose, we propose an extension VoronoiFVMExtendableFEMBaseExt which extends the capabilities of edgevelocities and bfacevelocities. They now provide the integrals of the velocity fields along Voronoi cell edges as computed by ExtendableFEMBase.

We also incorporated an example script which tests the fidelity of this extension and demonstrates the use case of working with two different grids: a flowgrid on which the velocity has been computed by ExtendableFEM.jl and a chemgrid onto which this velocity is interpolated to provide the input for the convection-diffusion system solved by VoronoiFVM.jl.

Mind that we still need the last pull request for ExtendableFEMBase.jl to go through so we can adjust the [compats] for VoronoiFVM.jl accordingly.

[j-fu]

Thanks for the PR!
We need ExtendableFEM(Base) in test/Project.toml and docs/Project.toml .

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 95.15152% with 8 lines in your changes missing coverage. Please review.

Project coverage is 85.84%. Comparing base (b176ec7) to head (740d8b9).
Report is 172 commits behind head on master.

Files with missing lines	Patch %	Lines
ext/VoronoiFVMExtendableFEMBaseExt.jl	94.36%	8 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #129      +/-   ##
==========================================
+ Coverage   85.33%   85.84%   +0.50%     
==========================================
  Files          24       26       +2     
  Lines        2803     3045     +242     
==========================================
+ Hits         2392     2614     +222     
- Misses        411      431      +20     

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

[j-fu]

The current test in Example240 basically tests the if the interpolation from a FE space gives the same result as the interpolation of the analytical function. However this is no useful example to clone for users (which is their main purpose; Yes, I know, some of mine are not that good either...)

Therefore I propose the following:

• Move the following to a test_feminterpolation.jl in the tests subdirectory. It seems that this can run entirely without the solve.

  @test norm(evelo_analytical .- evelo_fem, Inf) ≤ 1.0e-11
  @test norm(bfvelo_analytical .- bfvelo_fem, Inf) ≤ 1.0e-09

• Make the full_fem_demo the main in Example240. Possibly also compare to the analytical velocities. One could also test for the correct min/max.

[j-fu]

What are the kwargs... parameters doing ? They seem to be unused.

[j-fu]

Also please add docstrings to all newly introduced methods.
In particular, the main API methods are edgevelocities(grid, vel::FEVectorBlock) and
bfacevelocities(grid, vel::FEVectorBlock)? Their docstrings will be for users, whereas the others would go to into the internal docs.

I would care about putting them into the Documenter build.

[pjaap]

What are the kwargs... parameters doing ? They seem to be unused.

The new internal function _integrate_along_segments is not parameter free. It takes interpolate_eps and axisymmetric as kwargs.
We want to be able to set this from the top level and therefore we need to hand down these kwargs through several functions.

Are we missing any possible side effects of this change?

[pjaap]

@j-fu All new methods are now documented. The original "internal" integrate calls for the edgevelocities have now a proper description in a docstring, too.
However, they are documented in Postprocessing in the VoronoiFVM docs. This is probably not the optimal position.
Any suggestion where to put them?
The different methods are already dispatched, so reorganizing the docs is simple.

[Da-Be-Ru]

Things are still not quite done. Here's a short list of issues that need work or resolving:

• getting the pull request for ExtendableFEMBase.jl merged
• somehow, we still get a non-vanishing FVM divergence for the cylindrical case in the Example script despite the fact that the computed velocity fields seems to match with the analytical solution at least on the grid nodes
• we discussed actually brute-forcing the gFindLocal call in _integrate_along_segments in case it comes up empty after the first call - we should still test that
• there's still the question of where to put the docs for the internal integrate functions; they should be somewhere as this is a huge help for anyone who needs to understand the internals
• since we now have a version of the example where the FEM-computed velocity field resolves the analytical solution exactly, we might not even need the old main anymore; @j-fu should we still put this into a separate test somewhere? This would be a test of the edgevelocities and bfacevelocities functionality without relying on the FEM solve at all, but just on the interpolation which is slimmer and more stable, but less useful for actual users as you point out

Oh, and note to @chmerdon: if we had a HDIVBDM3 space, we could probably also resolve the quadratic case here 👀

[j-fu]

Things are still not quite done. Here's a short list of issues that need work or resolving:

• getting the pull request for ExtendableFEMBase.jl merged

Ok, this is done now.

• somehow, we still get a non-vanishing FVM divergence for the cylindrical case in the Example script despite the fact that the computed velocity fields seems to match with the analytical solution at least on the grid nodes

How about the maximum principle ? An the we also should cross-check the VoronoiFVM divergence caclulation

• we discussed actually brute-forcing the gFindLocal call in _integrate_along_segments in case it comes up empty after the first call - we should still test that

ok

• there's still the question of where to put the docs for the internal integrate functions; they should be somewhere as this is a huge help for anyone who needs to understand the internals

This should go to the internal API section, see https://j-fu.github.io/VoronoiFVM.jl/stable/internal/

• since we now have a version of the example where the FEM-computed velocity field resolves the analytical solution exactly, we might not even need the old main anymore; @j-fu should we still put this into a separate test somewhere? This would be a test of the edgevelocities and bfacevelocities functionality without relying on the FEM solve at all, but just on the interpolation which is slimmer and more stable, but less useful for actual users as you point out

I already created this test in my local copy. It will be available once I merge with your work, so you can delete this.

Oh, and note to @chmerdon: if we had a HDIVBDM3 space, we could probably also resolve the quadratic case here 👀

Nice!

[chmerdon]

There is no HDIVBDM3, yet, but there is HDIVRTk(2,3)... will also have a look at the example next week.

[chmerdon]

Concerning the testcase that does not work with usefem = true and Cylindrical2D... I think the velocity from the fem computation is the reconstruction of rv in that case, while the velocity for usefem = false or Cartesian2D is just v, right? Currently, it seems the FEM computes the divergence of (rv), which should be zero and is indeed zero, so only the divergence from the FVM is wrong. My guess is that the edgevelocities are probably computed incorrectly due to the wrong handling of the rv input.

[chmerdon]

My guess is that this axisymmetric switch is not passed properly through from the edgevelocities function to the segment integration. Moreover, I think we should also provide the option to give the v instead of r*v also in the FEM case.

[Da-Be-Ru]

My guess is that this axisymmetric switch is not passed properly through from the edgevelocities function to the segment integration. Moreover, I think we should also provide the option to give the v instead of r*v also in the FEM case.

You're correct, we forgot to handle to pass the reconst = true flag to the edgevelocities and bfacevelocities call for the cylindrical case. If it's not passed, we're using the wrong postprocessing kernel for the SegmentIntegrator.
With that fixed, the $$\mathrm{div}$$ evaluations seem to run through as expected.

I'll push a cleaned up version with some more changes applied later.

[pjaap]

An the we also should cross-check the VoronoiFVM divergence caclulation

I studied this function in order to understand the divergence problem and can say that it looks fine :)

[pjaap]

This should go to the internal API section, see https://j-fu.github.io/VoronoiFVM.jl/stable/internal/

Done.

[Da-Be-Ru]

I fixed the aforementioned error with the divergence calculation, incorporated the option to explicitly specify whether to perform an interpolation onto a reconstruction space, and cleaned up and restructured everything a bit to generate a neater doc output.

The only two things that seem to remain open in my view are thus:

1. Should we stay with the linear stagnation flow field in the cylindrical case or attempt something with a higher order reconstruction space? (I have not fooled around with this yet)
2. In the cylindrical, non-reconstructive case, the method yields very different edgevelocities, but apparently matching solutions for the species concentrations. I haven't looked into why this happens yet.

[pjaap]

Thanks for fixes and cleanup. I left some nitpicking comments in the code :)

[Da-Be-Ru]

Added comment as suggested.