TDycore polymorphism, part 3: separating concerns #215

jeff-cohere · 2021-11-15T23:19:01Z

Continuing in our journey (see #211 and #214):

This pull request defines interfaces for all important operations needed by our different numerical discretizations. Specifically:

TDy is now just a container with all the relevant information that describes a problem
The TDy struct contains a context pointer that is used to manage the specific implementations using the function pointers defined in the ops table.
The context pointer stores the WY, BDM, or MPFA-O implementation selected by TDySetDiscretization.
The calculation of boundary conditions (for pressure, temperature, velocity) and sources/sinks (forcings) have been separated into a Conditions type. They have been "vectorized" in the sense that they now can compute their values on multiple points.
All model data has been removed from the MaterialProps and CharacteristicCurves types, which now have reworked, vectorized interfaces. This allows different implementations to store material properties differently, instead of forcing them to have similar layouts. The MaterialProps and CharacteristicCurves interfaces now only compute model data.
Moving model data out of the above types required some changes in the IO system, in addition to those made in the specific numerical implementations.
There's a new EOS type that calculates properties of water.
Some source files have been moved into new directories:
- fe: the finite element discretizations and related utilities live here
- materials: EOS, CharacteristicCurves, and MaterialProp live here
- mpfao: the MPFA-O code lives here (not a new folder, but it has taken on new code)
Many functions have been removed from the public interface and made internal.
The MPFA-O mesh has been consolidated into a smaller number of source files, and it has been separated from TDy itself (because it's part of the MPFA-O implementation).
The Fortran interface has been reworked with vectorized functions for material properties, boundary conditions, and forcings, like its C counterpart. All Fortran-related source files now live in src/f90-mod.
Several memory leaks in the MPFA-O code have been plugged.
Bonus: source files in the library are now built in a deterministic order, which helps when you're trying to fix a lot of things in various places. :-)

In particular, there's a much stronger sense of an "interface" for TDycore. Instead of any piece doing whatever it wants at any point, we're moving toward a set of interfaces that perform specific tasks in specific contexts in a way that suggests a flow of information that is easier to understand.

So where are we?

Everything works.

I had to disable a test and some diagnostic output in transient_snes_mpfaof90 because it uses some low-level functions that arguably don't belong in a demo, like directly setting coefficients and extracting data, which implies knowledge of how things are indexed. Maybe we should discuss this when we have a chance.
I've disabled the mesh geometry reading/writing capability, since it makes assumptions about the mesh construction process that no longer hold.
The TH demos exhibited a very tricky problem that led to the addition of a set_dm_fields operation, required by all models, that set up the field layout on a DM before it's distributed. The way it was done before was not correct, but for some reason, we hadn't experienced any problems with it till now.

What's next?

The bad news is: not everything will be perfect after this PR goes in. A lot of cleanup remains, but much of it is within the separate numerical implementations, which lets different parts of our team adjust things without affecting others.

The good news: the changes in this PR make it easier for us to move in the direction of the DM-oriented problem definition that we started discussing in #211. Briefly: we attach functions to a DM for computing residuals, Jacobians, etc, and then we hand the DM over to the solvers. This allows us to make TDycore's interface smaller and tighter--the dycore produces a DM that gets used by the solvers directly.

Fixes #133
Fixes #196
Fixes #197
Fixes #22

This is a real mess at the moment, but it contains most of the essential changes. Now to dig our way back to sanity.

…ific stuff.

Also pausing to see whether I can get rid of an irritating build system "feature."

…lProp.

Next up: MPFAO!

jeff-cohere · 2021-11-29T17:40:05Z

Okay, I've fixed some issues I found in the higher-level driver interface, which drives the richards and th demos. Now these drivers run, but they get bad answers. I could go ahead and find out exactly what's going on, but it would be faster if someone who knew more about the driver interface could help me understand the assumptions that it makes.

These demos and the transfer of saved geometry info in the transient_snes_mpfaof90 demo are the only issues remaining, as far as I can tell. @bishtgautam , maybe we should discuss how to proceed.

jeff-cohere · 2021-11-30T18:32:27Z

The Richards driver tests now pass. The TH ones still fail.

Also, I found the source of the issue with reading in mesh geometry from a binary file: the new mesh construction process has already converted indices to compressed row format by the time the data is read, whereas the old construction process read in the geometry before doing that conversion.

jeff-cohere · 2021-12-01T18:54:29Z

@bishtgautam and I agreed that we can comment out the mesh geometry reading/writing stuff for now, as we may not actually need it.

The TH tests are the only remaining failures.

src/materials/tdycharacteristiccurves.c

bishtgautam · 2021-12-10T23:49:16Z

demo/transient/transient_snes_mpfaof90.F90

+!     call TDySetPorosityFunction(tdy,PorosityFunctionPFLOTRAN,ierr);
+!     CHKERRA(ierr);
+!
+!     do c = 1,ncell
+!        index(c) = c-1;
+!        call MaterialPropAlpha_PFLOTRAN(alpha(c))
+!        call MaterialPropM_PFLOTRAN(m(c))
+!     enddo
+!
+!     call TDySetCharacteristicCurveVanGenuchtenValuesLocal(tdy,ncell,index,m,alpha,ierr)
+!     CHKERRA(ierr);
+!
+!     call TDySetCharacteristicCurveMualemValuesLocal(tdy,ncell,index,m,ierr)
+!     CHKERRA(ierr);


Is there a new issue to add this functionality back in the future?

Yes--see #216

bishtgautam · 2021-12-10T23:49:35Z

demo/transient/transient_snes_mpfaof90.F90

-    mass_pre = mass_pre + liquid_mass(g)
-    enddo
+    ! TODO: Mass conservation diagnostic output is disabled for the moment.
+    !call TDyGetLiquidMassValuesLocal(tdy,nvalues,liquid_mass,ierr)


Is there a new issue to add this functionality back in the future?

Not yet. I think we should discuss how we'd like to do this and the similar stuff below. We need to be careful about what we guarantee to be true when we don't necessarily know the discretization (from outside the library). Alternatively, we could have functions that begin with TDyMPFAO for this, which would tie the discretization to the finite volume one, for example.

My point here is that we need to be able to extract the liquid mass, saturation, etc from TDycore. Since that functionality has been disabled in this PR, it would be good to track the issue. Though, the exact approach of how we go about adding this functionality back is still undecided at the moment. We can make a more generic issue of extracting data from the library. Does that make sense?

Yep. I've added #217

bishtgautam · 2021-12-10T23:49:44Z

demo/transient/transient_snes_mpfaof90.F90

@@ -468,8 +452,8 @@ program main

  end do

-  call TDyGetSaturationValuesLocal(tdy,nvalues,liquid_sat,ierr)
-  CHKERRA(ierr);
+  !call TDyGetSaturationValuesLocal(tdy,nvalues,liquid_sat,ierr)


Is there a new issue to add this functionality back in the future?

bishtgautam · 2021-12-10T23:50:12Z

include/private/tdymeshimpl.h

+// These don't work, and we'll likely get rid of them.
+//PETSC_INTERN PetscErrorCode TDyMeshReadGeometry(TDyMesh*,const char*);
+//PETSC_INTERN PetscErrorCode TDyMeshWriteGeometry(TDyMesh*,const char*);


Is there a new issue to add this functionality back in the future?

It depends on whether we need it. Is this just for saving time in debugging? At this point, we could make a proper effort to optimize the calculation of geometry so we don't need this feature.

…ect/TDycore into jeff-cohere/impls-refactor

codecov-commenter · 2021-12-16T17:07:11Z

Codecov Report

Merging #215 (fe06c7a) into master (b97463a) will increase coverage by 2.14%.
The diff coverage is 67.55%.

@@            Coverage Diff             @@
##           master     #215      +/-   ##
==========================================
+ Coverage   49.48%   51.63%   +2.14%     
==========================================
  Files           4        4              
  Lines         685      765      +80     
==========================================
+ Hits          339      395      +56     
- Misses        346      370      +24

Impacted Files	Coverage Δ
demo/transient/transient.c	`0.00% <0.00%> (ø)`
demo/steady/steady.c	`54.13% <72.20%> (+2.72%)`	⬆️
demo/th/th_driver.c	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b97463a...fe06c7a. Read the comment docs.

jeff-cohere added 30 commits October 25, 2021 13:50

Started factoring specific discretizations out of TDy.

d764715

This is a real mess at the moment, but it contains most of the essential changes. Now to dig our way back to sanity.

Pushing further. Things aren't getting better yet.

45caf1d

Factored out boundary conditions, sources/sinks, and more MPFA-O spec…

16f17f6

…ific stuff.

Working through MPFA-O mesh issues.

3903318

Factored out TDyEOS for density/viscosity/enthalpy calculations.

efef048

Adding needed parameters to impl functions.

ad47ed5

Also pausing to see whether I can get rid of an irritating build system "feature."

Fixing the order in which object files are compiled.

711ddca

Made some progress on understanding and reorganizing material models.

eb5cee9

Broke data out of MaterialProp type.

a6321c5

A suggested design for characteristic curves.

dc96251

Reimagined characteristic curves models and started prep for cleanup.

87f6905

Started on the long road back. Reworked Conditions to be like Materia…

6f944f1

…lProp.

Working through some issues with boundary conditions.

1e5e8ed

Documented function registry.

ea12571

Repairing finite element implementations.

e12150f

Worked through WY compilation errors, and fixed some oversights in CC.

0c8243d

Worked through material property issues and remaining FE stuff.

ffc8079

Next up: MPFAO!

Moving some things around and renaming things.

a604ff2

Started on Fortran. Going to skip for now.

0380d84

Continuing factor-out of MPFA-O mesh.

07a69c5

Consolidated MPFA-O mesh source and shook TDy out of it.

6bf4937

Got through most of tdympfao.c.

c4e0f70

Continuing on to solver-specific stuff.

9f311eb

Stopping for now.

ea16101

Sifting through and cleaning up some variable names.

1cade00

Waded through rest of MPFA-O compile errors.

1a722de

Worked through some remaining issues with material props and BCs.

305ceb1

Replaced porosity/permeability assignment logic in I/O system.

21ed75c

Finished code repairs to dycore. Next up: demos!

c29667d

Got through most of steady demo.

6471eff

Fixed some issues with the Richards driver.

59af1ca

jeff-cohere added 2 commits November 30, 2021 08:06

Fixed Richards driver tests.

c6f4f12

Got rid of some unnecessary allocations.

a0d512b

Some minor print changes.

a344e5e

jeff-cohere mentioned this pull request Dec 1, 2021

Post-215 work: support Fortran material properties/char curves functions with contexts #216

Open

Commented out functions for reading/writing mesh geometry.

5b3cce4

jeff-cohere force-pushed the jeff-cohere/impls-refactor branch from c83e37d to 5b3cce4 Compare December 3, 2021 18:53

jeff-cohere added 4 commits December 3, 2021 13:25

Fixed some small oversights in an attempt to chase down the last TH bug.

26fdc24

Committing debugging environment for TH.

7eb48a8

Added a new op, set_dm_fields, that sets field layouts for each model.

8f2d046

Removed debugging prints.

46a67f4

jeff-cohere mentioned this pull request Dec 10, 2021

Consolidate data structures between MFE and MPFA #22

Closed

bishtgautam reviewed Dec 10, 2021

View reviewed changes

src/materials/tdycharacteristiccurves.c Show resolved Hide resolved

src/materials/tdycharacteristiccurves.c Show resolved Hide resolved

src/materials/tdycharacteristiccurves.c Show resolved Hide resolved

bishtgautam reviewed Dec 10, 2021

View reviewed changes

jeff-cohere added 2 commits December 16, 2021 09:03

Adding else clauses in reponse to PR comments.

6216014

Merge branch 'jeff-cohere/impls-refactor' of github.com:TDycores-Proj…

fe06c7a

…ect/TDycore into jeff-cohere/impls-refactor

This was referenced Dec 16, 2021

Post-215 work: add support for retrieving liquid mass/saturation values directly from a model #217

Closed

Re-add functions for reading/writing mesh geometry #218

Open

bishtgautam self-requested a review December 16, 2021 17:32

bishtgautam assigned jeff-cohere Dec 16, 2021

bishtgautam approved these changes Dec 16, 2021

View reviewed changes

jeff-cohere merged commit f6d7fd0 into master Dec 16, 2021

jeff-cohere deleted the jeff-cohere/impls-refactor branch December 16, 2021 17:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TDycore polymorphism, part 3: separating concerns #215

TDycore polymorphism, part 3: separating concerns #215

jeff-cohere commented Nov 15, 2021 •

edited

Loading

jeff-cohere commented Nov 29, 2021 •

edited

Loading

jeff-cohere commented Nov 30, 2021

jeff-cohere commented Dec 1, 2021

bishtgautam Dec 10, 2021

jeff-cohere Dec 16, 2021

bishtgautam Dec 10, 2021

jeff-cohere Dec 16, 2021

bishtgautam Dec 16, 2021

jeff-cohere Dec 16, 2021

bishtgautam Dec 10, 2021

jeff-cohere Dec 16, 2021

bishtgautam Dec 10, 2021

jeff-cohere Dec 16, 2021

jeff-cohere Dec 16, 2021

codecov-commenter commented Dec 16, 2021 •

edited

Loading

TDycore polymorphism, part 3: separating concerns #215

TDycore polymorphism, part 3: separating concerns #215

Conversation

jeff-cohere commented Nov 15, 2021 • edited Loading

So where are we?

What's next?

jeff-cohere commented Nov 29, 2021 • edited Loading

jeff-cohere commented Nov 30, 2021

jeff-cohere commented Dec 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Dec 16, 2021 • edited Loading

Codecov Report

jeff-cohere commented Nov 15, 2021 •

edited

Loading

jeff-cohere commented Nov 29, 2021 •

edited

Loading

codecov-commenter commented Dec 16, 2021 •

edited

Loading