-
Notifications
You must be signed in to change notification settings - Fork 91
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use namespace for global variables 'mesh' and 'dump' #1470
Conversation
9d18cf3
to
b145852
Compare
Sorry missed that this is going into |
The
|
#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). |
Righto, in that case it does make sense to rebase this on next. Will do now... |
3c591fb
to
015fb4f
Compare
I've rebased this PR on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
015fb4f
to
d0401ee
Compare
@ZedThree I think I've addressed all your comments except the one about 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'.
Thanks! I'll look through them now.
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. |
@@ -606,10 +606,10 @@ void SlepcSolver::monitor(PetscInt its, PetscInt nconv, PetscScalar eigr[], | |||
output<<formatEig(reEigBout,imEigBout)<<endl; | |||
if(eigenValOnly){ | |||
simtime=reEigBout; | |||
dump.write(); | |||
bout::globals::dump.write(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)?
Looks good to me, but @d7919 had a bunch of comments too -- are you happy those are resolved? |
include/invert_laplace.hxx
Outdated
@@ -192,7 +192,7 @@ public: | |||
* | |||
* @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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Have you checked that we can compile and run an appropriately updated example without using the |
Might be wise to merge in |
Avoids potential issues if order of FCIMap member declarations changes in the future.
@d7919 The integrated tests run without needing any update. Examples and user code should not need updating either. I've just checked |
and also for LaplaceFactory::createLaplacian. LaplaceFactory uses bout::globals::mesh if the input 'Mesh* mesh_in' is nullptr.
bout::globals
containingMesh* mesh
andDatafile dump
.bout.hxx
include statementusing namespace bout::globals
so no changes are needed to user code.mesh
pointer in several places in the library code.bout::globals::mesh
) is still used in many places as a default argument or ifnullptr
is passed as aMesh*
.bout::globals::mesh
are: the solver code (see Solver depends on global mesh #1471);FieldData
(which should be refactored, possibly mergingFieldData
andField
, see also MakeField
more powerful #897).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.using namespace bout::globals
because they use the global mesh.test-petsc_laplace
, just getting it to work and removing the binary grid file.This implements the suggestion in #1468.