Skip to content
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

LaplaceHypre3d statistics output #2260

Conversation

johnomotani
Copy link
Contributor

Writes an average iteration count from LaplaceHypre3d to the dump files, if Laplacian::create() is called with Solver and Datafile arguments. The average iteration count for each output step will also be printed if the command line flag --verbose (or -v) is passed.

@ZedThree @bendudson I've added extra arguments to the Laplacian constructor to implement this. What do you think of the design?

Allows Laplacian implementations to define monitors and add output to
dump files if users pass a Solver and a Datafile to the
Laplacian::create() method.
If the Laplacian is constructed with Solver and Datafile arguments, it
will write an average iteration count to the Datafile.
@johnomotani johnomotani added feature proposal A code/feature outline proposal work in progress Not ready for merging labels Mar 9, 2021
src/invert/laplace/impls/hypre3d/hypre3d_laplace.hxx Outdated Show resolved Hide resolved
public:
static constexpr auto type_name = "Laplacian";
static constexpr auto section_name = "laplace";
static constexpr auto option_name = "type";
static constexpr auto default_type = LAPLACE_CYCLIC;

ReturnType create(Options* options = nullptr, CELL_LOC loc = CELL_CENTRE,
Mesh* mesh = nullptr) {
Mesh* mesh = nullptr, Solver* solver = nullptr,
Datafile* dump = nullptr) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a note that when we get the output refactoring in, this will be a bit nicer as we'd be able to drop the Datafile

@ZedThree
Copy link
Member

I'm always a little cautious about adding more arguments to constructors, but I think this is probably the best way of doing things.

Another option might be to just make the monitor available and then up to the user to add it. That would end up looking something like:

auto laplacian = Laplacian::create(options);
laplacian.monitor.outputVars(dump);       // This would need to be added
solver->addMonitor(&laplacian.monitor);

which is not a terrible amount of boilerplate for user code, but still, maybe not quite as nice as just

auto laplacian = Laplacian::create(options, CELL_CENTRE, mesh, solver, dump);

With the warning in the constructor, this feature is actually discoverable, where it wouldn't be with the user-side monitor registration.

@johnomotani
Copy link
Contributor Author

A slightly different approach (while we can still break interfaces for v5) might be to pass a PhysicsModel to the Laplacian constructor and let the Laplacian get Mesh*, Solver* and Datafile* from there (maybe add getter methods if necessary). Then we wouldn't need to increase the number of arguments to the constructor, just replace the Mesh* with a PhysicsModel*. I guess in practice nobody is using the mesh argument, so it shouldn't be a big deal to change.

@ZedThree
Copy link
Member

Definitely something to consider! I'm happy with this approach though

@dschwoerer
Copy link
Contributor

I would prefer to not require a full physics model to use the Laplacian. I have used the Laplacian in the past without actually having a physics model, although I used only one mesh, so maybe that would be fine for that use case.
But I think it is a bit overkill to create a physics model just to be able to bundle three pointers.

At least for the python interface, I think three separate pointers are much nicer 👍

@johnomotani
Copy link
Contributor Author

Good points @ZedThree @dschwoerer. Lets stick with the interface in this PR 👍

@dschwoerer
Copy link
Contributor

auto laplacian = Laplacian::create(options);
laplacian.monitor.outputVars(dump);       // This would need to be added
solver->addMonitor(&laplacian.monitor);

could be simplified to:

auto laplacian = Laplacian::create(options);
laplacian.monitor.outputVars(dump, solver);       // This would need to be added

and in the case that solver==nullptr (default) the monitor is not added. Might even be a bit easier to read, and would allow to be called later - e.g. if I notice that the simulation is starting to cause issues - but I have no strong feelings about this ...

@johnomotani johnomotani removed the work in progress Not ready for merging label Mar 15, 2021
@johnomotani
Copy link
Contributor Author

@bendudson - I'm not sure if the branch in this PR ever got merged in to anything - if not I think it would be useful to merge into whatever the current GPU branch is...

@bendudson bendudson changed the base branch from next-hypre-outerloop-cuda to next-hypre-outerloop-cuda-merged September 28, 2021 18:02
Squash some warnings about unused variables (Solver and Datafile)
in Laplacian constructors.
@bendudson bendudson merged commit 351577a into next-hypre-outerloop-cuda-merged Sep 29, 2021
@bendudson bendudson deleted the next-hypre-outerloop-cuda-hypre3d-stats-output branch September 29, 2021 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature proposal A code/feature outline proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants