Use namespace for global variables 'mesh' and 'dump' by johnomotani · Pull Request #1470 · boutproject/BOUT-dev

johnomotani · 2018-12-19T14:28:39Z

Create the namespace bout::globals containing Mesh* mesh and Datafile dump.
In bout.hxx include statement using namespace bout::globals so no changes are needed to user code.
Remove references to global mesh pointer in several places in the library code.
The global pointer (renamed to bout::globals::mesh) is still used in many places as a default argument or if nullptr is passed as a Mesh*.
The last places depending on bout::globals::mesh are: the solver code (see Solver depends on global mesh #1471); FieldData (which should be refactored, possibly merging FieldData and Field, see also Make Field more powerful #897).
SlepcSolver uses bout::globals::dump directly because it needs to write output without calling BoutMonitor. Seems acceptable to leave this as a special case because SlepcSolver needs a different output scheme as an eigenvalue rather than initial-value solver.
The only changes to the tests are that the unit tests need to include using namespace bout::globals because they use the global mesh.
There is an unrelated change to test-petsc_laplace, just getting it to work and removing the binary grid file.

This implements the suggestion in #1468.

bendudson · 2018-12-23T15:49:10Z

Sorry missed that this is going into forward_declares rather than next. Can it go into next then merge next into forward_declares?

johnomotani · 2018-12-28T14:24:03Z

The namespace_globals branch includes a bunch of commits from forward_declares. It uses at least a few of the changes, which we could 'rebase' away, but if #1464 is nearly ready it would be easier not to:

including physicsmodel.hxx in solver.cxx instead of solver.hxx
moving Field::getMesh() implementation from .hxx to .cxx

d7919 · 2018-12-30T19:16:12Z

#1464 was kind of an experiment to see if there were any immediate gains -- I think there are some (both in terms of compilation time as well as flexibility) but the commit history might be a bit noisier than required as I approached this in a roundabout way. @ZedThree has looked into tools that might be able to automate some of these include changes (although the compilation time gains would not have been achieved with this -- it required some insight to see how to make a gain).

johnomotani · 2019-01-02T10:51:52Z

Righto, in that case it does make sense to rebase this on next. Will do now...

johnomotani · 2019-01-02T13:41:14Z

I've rebased this PR on next instead of forward_declares now, and tidied up the commit history a little bit. I think the PR should be ready to merge now, unless anyone has more comments?

ZedThree

A super minor comment: in some places you've done:

Mesh* mesh = f.getMesh()

and in others

Mesh* localmesh = f.getMesh();

Is it worth picking one and being consistent (answer might be no!)? I only mention this as we're touching all these places

Also set ny=1, myg=1.

Forward declare class Mesh instead. Requires explicitly including mesh.hxx in several other files, plus a few other extra explicit includes.

Handle the nullptr case in the initializer list instead.

nullptr is handled by the base Laplacian class constructor and converted to bout::globals::mesh.

Use bout::globals::mesh in Coordinates, and test_coordinates

Forward declare Field2D, Field3D, Vector2D and Vector3D in datafile.hxx to avoid circular dependencies due to including datafile.hxx in globals.hxx.

johnomotani · 2019-01-02T22:42:49Z

@ZedThree I think I've addressed all your comments except the one about Mesh* naming consistency.

BTW I made the changes as a rebase because there are already rather a lot of commits in this PR. Do we have a style preference for (a) 'fixing' existing commits or (b) adding new commits with small changes/fixes to a PR?

Update SlepcSolver to use 'bout::globals::dump' instead of 'dump'.

BOUTMAIN is defined in physicsmodel.hxx, so a file using it will usually include 'using namespace bout::globals' from physicsmodel.hxx, but in case the 'using' statement is explicitly excluded using the BOUT_NO_USING_NAMESPACE_BOUTGLOBALS macro or if in future the 'using' statement is removed, use the full name 'bout::globals::dump' instead of 'dump'.

ZedThree · 2019-01-03T16:30:28Z

Thanks! I'll look through them now.

BTW I made the changes as a rebase because there are already rather a lot of commits in this PR. Do we have a style preference for (a) 'fixing' existing commits or (b) adding new commits with small changes/fixes to a PR?

I prefer (b) as it's then possible to review just those commits, although if it's something like a typo in the last commit and I catch it early, I sometimes amend and force-push.

ZedThree · 2019-01-03T16:43:01Z

      if(eigenValOnly){
        simtime=reEigBout;
-        dump.write();
+        bout::globals::dump.write();


Not for this PR, but it might be nice for SlepcSolver to have it's own output file (which is bout::globals::dump by default)?

ZedThree · 2019-01-03T16:49:59Z

Looks good to me, but @d7919 had a bunch of comments too -- are you happy those are resolved?

d7919 · 2019-01-29T12:59:15Z

   * @param[in] opt  The options section to use. By default "laplace" will be used
   */
-  static Laplacian *create(Options *opt = nullptr, const CELL_LOC loc = CELL_CENTRE, Mesh *mesh_in = mesh);
+  static Laplacian *create(Options *opt = nullptr, const CELL_LOC loc = CELL_CENTRE, Mesh *mesh_in = bout::globals::mesh);


Should mesh_in really be nullptr here? Presumably this is passed to the constructor noted above (or equivalent) which can then choose what to do if it receives nullptr?

Agreed -- nullptr is a better default, it's what we do in other bits of the code, as well as giving us some space to potential remove the global mesh entirely.

We can't just pass nullptr here because LaplaceFactory uses mesh_in to check whether we can use a serial solver

BOUT-dev/src/invert/laplace/laplacefactory.cxx

Lines 46 to 47 in 08cc9f2

if(mesh_in->firstX() && mesh_in->lastX()) {

// Can use serial algorithm

I think this was originally to allow the default Laplace solver to switch between serial_tri and spt depending on whether a serial or parallel solver was needed, but now the default is cyclic in either case. I think it would be better now to allow any Laplace solver to be created, and move the check into the constructor of the serial solvers. Does that sound reasonable, and should I do it here or create a separate PR?

If LaplaceFactory gets a nullptr, it could just fetch the global mesh though?

LaplaceFactory should get rewritten anyway (e.g. using GenericFactory), so I would leave that to another PR

d7919 · 2019-01-29T13:09:20Z

Have you checked that we can compile and run an appropriately updated example without using the using statements?

ZedThree · 2019-02-01T13:56:05Z

Might be wise to merge in next now 4.2.1 is in and verify everything still works

Avoids potential issues if order of FCIMap member declarations changes in the future.

johnomotani · 2019-02-04T14:06:19Z

Have you checked that we can compile and run an appropriately updated example without using the using statements?

@d7919 The integrated tests run without needing any update. Examples and user code should not need updating either. I've just checked examples/blob2d to be sure.

and also for LaplaceFactory::createLaplacian. LaplaceFactory uses bout::globals::mesh if the input 'Mesh* mesh_in' is nullptr.

johnomotani added the proposal A code/feature outline proposal label Dec 19, 2018