Consolidating and simplifying command-line options #195

jeff-cohere · 2021-08-10T16:31:27Z

This PR gathers our command line options, elaborating on TDyOptions and adding features for specifying boundary condition functions and the like. I hope it's easier to understand.

Also, I'm forcing us to be more specific about how we specify a mesh, because our rules to date have been confusing. Here are the new rules:

If TDySetDM is called before TDySetFromOptions in a demo/driver, the DM passed to the dycore in that function is the mesh, period. It can't be overridden. We should only use this function in programs that use a fixed geometry, or that generate their own geometry without leaning on TDySetFromOptions.
If TDySetDM is not called before TDySetFromOptions, the user must specify exactly one of -tdy_read_mesh <filename> or -tdy_generate_mesh. If -tdy_generate_mesh is given, the properties of the mesh are extracted from our existing mesh-related command line options (though, as I mention in a comment on this PR, I believe we should eliminate our own options and rely on the options that PETSc provides for generating DMPlexes, for simplicity and to leverage the work of the PETSc team).
When TDySetFromOptions returns successfully, TDycore has a distributed mesh with the right overlapping cells and all that--there's no need for anything else to be done.

With these rules, it's always clear what the geometry is. This is good, because the word "obvious" is meaningless in every context in which anything matters. :-)

There's still work to be done. PR #188 proposes a lot of new options.

jeff-cohere · 2021-08-10T16:32:41Z

@bishtgautam , can you add other people who you believe would have an opinion on this stuff? The FE folks will eventually care, I think, but this is still just a work in progress at the moment.

knepley

This looks simpler and more understandable to me.

nocollier · 2021-08-10T17:05:33Z

I like the logic also. One question that wasn't clear to me from a quick look over the changes: if TDySetDM is called, and then an option is passed in for a mesh (violating rulle 1), will Options throw an error?

jeff-cohere · 2021-08-10T17:08:49Z

I like the logic also. One question that wasn't clear to me from a quick look over the changes: if TDySetDM is called, and then an option is passed in for a mesh (violating rulle 1), will Options throw an error?

Yes, in the name of specificity. If this is too irritating and we'd rather use the option to override the DM supplied with TDySetDM, that's fine, but this is where I started. If you look at the changes in tdycore.c, you can see how these rules are enforced in TDySetFromOptions (and also TDySetDM).

bishtgautam · 2021-08-10T17:13:34Z

though, as I mention in a comment on this PR, I believe we should eliminate our own options and rely on the options that PETSc provides for generating DMPlexes, for simplicity and to leverage the work of the PETSc team

What is PETSc's option to read a mesh?

jeff-cohere · 2021-08-10T17:20:33Z

though, as I mention in a comment on this PR, I believe we should eliminate our own options and rely on the options that PETSc provides for generating DMPlexes, for simplicity and to leverage the work of the PETSc team

What is PETSc's option to read a mesh?

If we want to read a mesh, we can use our own -tdy_read_mesh option. But if we specify -tdy_generate_mesh, we can fall back on the DM and DMPlex-related options supported by DMSetFromOptions (see PR #188 for a list)

bishtgautam · 2021-08-10T17:40:08Z

So, instead of options in https://github.com/TDycores-Project/TDycore/blob/master/src/tdydm.c#L19-L53, use the following?

-dm_plex_box_dim <dim>	- Set the topological dimension
-dm_plex_box_simplex <bool>	- PETSC_TRUE for simplex elements, PETSC_FALSE for tensor elements
-dm_plex_box_lower <x,y,z>	- Specify lower-left-bottom coordinates for the box
-dm_plex_box_upper <x,y,z>	- Specify upper-right-top coordinates for the box
-dm_plex_box_faces <m,n,p>	- Number of faces in each linear direction
-dm_plex_box_bd <bx,by,bz>	- Specify the DMBoundaryType for each direction
-dm_plex_box_interpolate <bool>	- PETSC_TRUE turns on topological interpolation (creating edges and faces)

jeff-cohere · 2021-08-10T17:43:14Z

So, instead of options in https://github.com/TDycores-Project/TDycore/blob/master/src/tdydm.c#L19-L53, use the following?
...

Yes, that's what I'm advocating. The benefit is that recent changes to PETSc (which we now have via the latest update) now make all these options available, and we could grab any new ones that appear without adding our own.

bishtgautam · 2021-08-10T17:44:35Z

@jeff-cohere I agree with your proposal. Let's use PETSc options.

jeff-cohere · 2021-08-10T17:48:40Z

Cool. I'll get rid of the TDycore-specific mesh generation options and retrofit the demos to use the PETSc ones, and I'll let everyone know if I hit any snags.

knepley · 2021-08-10T18:14:42Z

@bishtgautam For reading in a mesh, you can use -dm_plex_filename.

The only demo using this function was mpfao, which has been modified not to need it. Note the PETSc -dm_plex_* functions used as demo arguments.

jedbrown · 2021-08-10T22:07:28Z

include/private/tdymaterialpropertiesimpl.h

+PETSC_INTERN PetscErrorCode TDyConstantSoilDensityFunction(TDy,PetscReal*,PetscReal*,void*);
+PETSC_INTERN PetscErrorCode TDyConstantSoilSpecificHeatFunction(TDy,PetscReal*,PetscReal*,void*);
+PETSC_INTERN PetscErrorCode TDyConstantPermeabilityFunction(TDy,PetscReal *,PetscReal *,void*);
+PETSC_INTERN PetscErrorCode TDyConstantThermalConductivityFunction(TDy,PetscReal *,PetscReal *,void*);
+PETSC_INTERN PetscErrorCode TDyConstantPorosityFunction(TDy,PetscReal *,PetscReal *,void*);


Not pertinent to the name change, but we may want to make these vectorizable, taking arguments (TDy, PetscInt, const PetscReal *, PetscReal *, void *).

That's a good idea and one we should adopt.

jedbrown · 2021-08-10T22:09:20Z

src/tdyregression.c

@@ -201,7 +201,7 @@ PetscErrorCode TDyRegressionOutput(TDy tdy, Vec U) {
  ierr = VecRestoreArray(U,&u_p); CHKERRQ(ierr);
  ierr = VecRestoreArray(U_pres,&pres_p); CHKERRQ(ierr);

-  if (tdy->mode == TH) {
+  if (tdy->options.mode == TH) {


Also not pertinent to this step, but it would be nice to use callbacks instead of if statements, which will become switch statements if there are ever more than two modes.

Can you give an example of what you mean by this?

For example, the mode could have a tdy->diagnostics(tdy, ...) that computes/reports any diagnostics specific to that mode. This saves having a sequence of switch statements with possibly fragile carried dependencies.

Ah, okay. A "virtual table". Yes, that makes sense.

codecov-commenter · 2021-08-10T22:55:52Z

Codecov Report

Merging #195 (8c5c533) into master (b7a410b) will increase coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #195      +/-   ##
==========================================
+ Coverage   67.73%   67.81%   +0.07%     
==========================================
  Files           7        7              
  Lines        1221     1224       +3     
==========================================
+ Hits          827      830       +3     
  Misses        394      394

Impacted Files	Coverage Δ
demo/mpfao/mpfao.c	`90.30% <100.00%> (+0.15%)`	⬆️
demo/richards/richards_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 b7a410b...8c5c533. Read the comment docs.

jeff-cohere · 2021-08-10T22:59:30Z

I don't think this covers all the ground mentioned in #188, but things are working, and I kinda don't want to make this PR much bigger. @bishtgautam , let me know if you'd like me to address anything else. @jedbrown , I made an issue for "vectorizing" our functions.

jeff-cohere added 6 commits August 9, 2021 12:32

Pull all options into TDySetFromOptions.

9cd1391

Removed default_ prefix from options.

58c3d96

Renamed TDyXYZFunctionDefault -> TDyConstantXYZFunction (material props)

ff62b36

Reorganized options into TDyOptions container and added some new ones.

82b7210

Touched up boundary condition args.

39f8873

Adding mesh generation/reading options. Not finished yet.

8a6171d

jeff-cohere added the cleanup Refactor code label Aug 10, 2021

jeff-cohere requested a review from bishtgautam August 10, 2021 16:31

Fixed a failing test.

1de8f72

knepley approved these changes Aug 10, 2021

View reviewed changes

jeff-cohere added 2 commits August 10, 2021 14:29

We now use PETSc's mesh args for TDySetFromOptions.

8f4433a

Eliminated TDyCreateDM.

25c73ca

The only demo using this function was mpfao, which has been modified not to need it. Note the PETSc -dm_plex_* functions used as demo arguments.

jedbrown approved these changes Aug 10, 2021

View reviewed changes

jeff-cohere mentioned this pull request Aug 10, 2021

Make material property, boundary condition functions vectorizable #196

Closed

Updated richards and th regression tests.

8c5c533

jeff-cohere changed the title ~~[WIP] Consolidating and simplifying command-line options~~ Consolidating and simplifying command-line options Aug 10, 2021

bishtgautam approved these changes Aug 10, 2021

View reviewed changes

jeff-cohere mentioned this pull request Aug 10, 2021

Rework TDycore with polymorphic "ops" tables #197

Closed

jeff-cohere merged commit 3181b1a into master Aug 10, 2021

jeff-cohere deleted the jeff-cohere/tdy-set-from-options branch August 10, 2021 23:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consolidating and simplifying command-line options #195

Consolidating and simplifying command-line options #195

jeff-cohere commented Aug 10, 2021 •

edited

Loading

jeff-cohere commented Aug 10, 2021

knepley left a comment

nocollier commented Aug 10, 2021

jeff-cohere commented Aug 10, 2021

bishtgautam commented Aug 10, 2021

jeff-cohere commented Aug 10, 2021

bishtgautam commented Aug 10, 2021

jeff-cohere commented Aug 10, 2021

bishtgautam commented Aug 10, 2021

jeff-cohere commented Aug 10, 2021

knepley commented Aug 10, 2021

jedbrown Aug 10, 2021

jeff-cohere Aug 10, 2021

jedbrown Aug 10, 2021

jeff-cohere Aug 10, 2021

jedbrown Aug 10, 2021

jeff-cohere Aug 10, 2021

codecov-commenter commented Aug 10, 2021

jeff-cohere commented Aug 10, 2021

Consolidating and simplifying command-line options #195

Consolidating and simplifying command-line options #195

Conversation

jeff-cohere commented Aug 10, 2021 • edited Loading

jeff-cohere commented Aug 10, 2021

knepley left a comment

Choose a reason for hiding this comment

nocollier commented Aug 10, 2021

jeff-cohere commented Aug 10, 2021

bishtgautam commented Aug 10, 2021

jeff-cohere commented Aug 10, 2021

bishtgautam commented Aug 10, 2021

jeff-cohere commented Aug 10, 2021

bishtgautam commented Aug 10, 2021

jeff-cohere commented Aug 10, 2021

knepley commented Aug 10, 2021

jedbrown Aug 10, 2021

Choose a reason for hiding this comment

jeff-cohere Aug 10, 2021

Choose a reason for hiding this comment

jedbrown Aug 10, 2021

Choose a reason for hiding this comment

jeff-cohere Aug 10, 2021

Choose a reason for hiding this comment

jedbrown Aug 10, 2021

Choose a reason for hiding this comment

jeff-cohere Aug 10, 2021

Choose a reason for hiding this comment

codecov-commenter commented Aug 10, 2021

Codecov Report

jeff-cohere commented Aug 10, 2021

jeff-cohere commented Aug 10, 2021 •

edited

Loading