Pvcode + Cvode improvements #2889
Conversation
dschwoerer
commented
Mar 19, 2024
dschwoerer
commented
- Exposes more options to the user.
- Allows to dump if the solver fails. This can be useful for figuring out why the solver is slow. Currently only for pvode, but should also be possible to implement this for cvode (for cvode we can use a documented API)
Use the new track feature (better name required) to dump the different components of the ddt() as well as the residuum for the evolved fields.
This keeps track of all the changes done to the field and stores them to a OptionsObject.
This keeps track of all the changes done to the field and stores them to a OptionsObject.
| @@ -683,4 +683,12 @@ inline T floor(const T& var, BoutReal f, const std::string& rgn = "RGN_ALL") { | |||
|
|
|||
| #undef FIELD_FUNC | |||
|
|
|||
| template <typename T, typename = bout::utils::EnableIfField<T>, class... Types> | |||
| inline T setName(T&& f, const std::string& name, Types... args) { | |||
There was a problem hiding this comment.
I think most other setters like this are member functions
There was a problem hiding this comment.
To make this a member function, make it virtual:
field.hxx:
virtual Field& setName(std::string name) {
self->name = std::move(name);
return *self;
}field2d.hxx:
Field2D& setName(std::string name) override {
Field::setName(name);
return *self;
}Formatting will have to be done at the calling site, but I think that's fine
|
My worry here is the tracking storing a lot of data if it's storing the full |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
src/solver/impls/pvode/pvode.cxx
Outdated
| } | ||
| pvode_load_data_f3d(evolve_bndrys, ffs, | ||
| prefix == "pre_"s ? udata : N_VDATA(cv_mem->cv_acor)); | ||
| } |
There was a problem hiding this comment.
warning: variable name 'f' is too short, expected at least 3 characters [readability-identifier-length]
for (auto& f : f3d) {
^
src/solver/impls/pvode/pvode.cxx
Outdated
| f.F_var->enableTracking(fmt::format("ddt_{:s}", f.name), debug); | ||
| setName(*f.var, f.name); | ||
| } | ||
| run_rhs(simtime); |
There was a problem hiding this comment.
warning: variable name 'f' is too short, expected at least 3 characters [readability-identifier-length]
for (auto& f : f3d) {
^
It could be, but I don't think it is needed. I would personally keep it enabled for production runs, as it has a runtime switch, that is disabled by default. And I would also keep it like this, at least by default. If a simulation crashes, it can be very useful to immediately get why it failed (this is currently the only use of the name tracking). Having to restart + queue again, just to get to the point where it crashes can be very time consuming, and the cost should be really negligable, if disabled. I did notice a slow down in my simulations with this enabled, but that was when it was enabled all the time and the names did not get reset, thus the names have been growing exponentially. |
|
On 3/19/24 16:34, Peter Hill wrote:
***@***.**** commented on this pull request.
------------------------------------------------------------------------
In include/bout/field.hxx
<#2889 (comment)>:
> @@ -683,4 +683,12 @@ inline T floor(const T& var, BoutReal f, const std::string& rgn = "RGN_ALL") {
#undef FIELD_FUNC
+template <typename T, typename = bout::utils::EnableIfField<T>, class... Types>
+inline T setName(T&& f, const std::string& name, Types... args) {
I think most other setters like this are member functions
But then they return either Field, or need to be overwritten.
|
src/solver/impls/pvode/pvode.cxx
Outdated
| @@ -293,6 +353,49 @@ | |||
| // Check return flag | |||
| if (flag != SUCCESS) { | |||
| output_error.write("ERROR CVODE step failed, flag = {:d}\n", flag); | |||
| CVodeMemRec* cv_mem = (CVodeMem)cvode_mem; | |||
There was a problem hiding this comment.
warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
CVodeMemRec* cv_mem = (CVodeMem)cvode_mem;
^
src/solver/impls/pvode/pvode.cxx
Outdated
| for (const auto& prefix : {"pre_"s, "residuum_"s}) { | ||
| std::vector<Field3D> list_of_fields{}; | ||
| std::vector<bool> evolve_bndrys{}; | ||
| for (const auto& f : f3d) { |
There was a problem hiding this comment.
warning: variable name 'f' is too short, expected at least 3 characters [readability-identifier-length]
for (const auto& f : f3d) {
^
src/solver/impls/pvode/pvode.cxx
Outdated
| prefix == "pre_"s ? udata : N_VDATA(cv_mem->cv_acor)); | ||
| } | ||
|
|
||
| for (auto& f : f3d) { |
There was a problem hiding this comment.
warning: variable name 'f' is too short, expected at least 3 characters [readability-identifier-length]
for (auto& f : f3d) {
^
src/solver/impls/pvode/pvode.cxx
Outdated
| } | ||
| run_rhs(simtime); | ||
|
|
||
| for (auto& f : f3d) { |
There was a problem hiding this comment.
warning: variable name 'f' is too short, expected at least 3 characters [readability-identifier-length]
for (auto& f : f3d) {
^
|
The cuda build fails with: The relevant code is: (*tracking)[outname].setAttributes({
{"operation", operation},
#if BOUT_USE_TRACK
{"rhs.name", change.name},
#endif
});Is gcc9.4 not able to use setAttributes? |
We do try to completely remove debugging features for production runs, see
This is true, but I would really like to understand the costs. It looks like it's storing potentially hundreds or thousands of copies of full fields, so although it looks like a really interesting and useful feature, it would be good to have a good idea of the costs.
Is that with the current form?
It should almost certainly return |
If at all, I would be in favor of adding a new configure flag. I use this in runs with CHECK=0.
It is only storing fields, if the solver has failed. Unless that happens, no additional storage is used. In the case the solver crashes, I think the cost is really negligible. This is only done once, if the solver decided it cannot continue. For hermes-2, the dump file contains 53 But if BOUT++ runs out of memory here, it would not be much worse then if not - the simulation finishes anyway.
No, currently it is not noticeable. I activate it for all my runs.
That is exactly what I tried: diff --git a/include/bout/field.hxx b/include/bout/field.hxx
index 04035f5b7..488aecaab 100644
--- a/include/bout/field.hxx
+++ b/include/bout/field.hxx
@@ -84,6 +84,14 @@ public:
return *this;
}
+ template <class... Types>
+ inline Field& setName(const std::string& name, Types... args) {
+#if BOUT_USE_TRACK
+ this->name = fmt::format(name, args...);
+#endif
+ return *this;
+ }
+
std::string name;
#if CHECK > 0
@@ -683,19 +691,4 @@ inline T floor(const T& var, BoutReal f, const std::string& rgn = "RGN_ALL") {
#undef FIELD_FUNC
-template <typename T, typename = bout::utils::EnableIfField<T>, class... Types>
-inline void setName(T& f, const std::string& name, Types... args) {
-#if BOUT_USE_TRACK
- f.name = fmt::format(name, args...);
-#endif
-}
-
-template <typename T, typename = bout::utils::EnableIfField<T>, class... Types>
-inline T setName(T&& f, const std::string& name, Types... args) {
-#if BOUT_USE_TRACK
- f.name = fmt::format(name, args...);
-#endif
- return f;
-}
-
#endif /* FIELD_H */
diff --git a/src/mesh/coordinates.cxx b/src/mesh/coordinates.cxx
index 32774d622..643e148b1 100644
--- a/src/mesh/coordinates.cxx
+++ b/src/mesh/coordinates.cxx
@@ -1542,7 +1542,7 @@ Field3D Coordinates::Grad_par(const Field3D& var, CELL_LOC outloc,
TRACE("Coordinates::Grad_par( Field3D )");
ASSERT1(location == outloc || outloc == CELL_DEFAULT);
- return setName(::DDY(var, outloc, method) * invSg(), "Grad_par({:s})", var.name);
+ return (::DDY(var, outloc, method) * invSg()).setName("Grad_par({:s})", var.name);
}
/////////////////////////////////////////////////////////
@@ -1601,7 +1601,7 @@ Field3D Coordinates::Div_par(const Field3D& f, CELL_LOC outloc,
f_B.yup(i) = f.yup(i) / Bxy_floc.yup(i);
f_B.ydown(i) = f.ydown(i) / Bxy_floc.ydown(i);
}
- return setName(Bxy * Grad_par(f_B, outloc, method), "Div_par({:s})", f.name);
+ return (Bxy * Grad_par(f_B, outloc, method)).setName("Div_par({:s})", f.name);
}
/////////////////////////////////////////////////////////
diff --git a/src/solver/impls/pvode/pvode.cxx b/src/solver/impls/pvode/pvode.cxx
index db28f64d8..c36985ae0 100644
--- a/src/solver/impls/pvode/pvode.cxx
+++ b/src/solver/impls/pvode/pvode.cxx
@@ -376,7 +376,7 @@ BoutReal PvodeSolver::run(BoutReal tout) {
for (auto& f : f3d) {
f.F_var->enableTracking(fmt::format("ddt_{:s}", f.name), debug);
- setName(*f.var, f.name);
+ f.var->setName(f.name);
}
run_rhs(simtime);gcc14 complains with: |
gcc 9.4 is unable to correctly parse the construction for the function argument, if name.change is used directly. First making a copy seems to work around that issue.
|
@dschwoerer After discussing this with @bendudson, we're quite concerned about the potential overhead of the temporary field dumping mechanism, both in terms of memory and performance, for what seems like a small benefit when debugging. We'd really like to see some performance analysis and scaling to see it doesn't have an affect when disabled. I'm also worried that we'd need to see Could something similar not be achieved with a custom monitor instead? Exposing more CVODE options to the user seems very straightforward and useful, so maybe that could be split out? |
But the memory overhead is only if the solver fails. Do you still think at that point that is an issue?
I can certainly run blob2d for this branch and next. But I am certain there are no significant differences, thus I haven't done it. Would you like to see something else? Would just the BOUT++ internal timings sufficient? Would you like mpi runs?
Sure, having them makes things more easily readable. But that is optional, and I can certainly add some if you think this is worth merging an wanted before merging. It would make
I would not know how. If you can explain how to, I am happy to change the design.
I can split them out 👍 |
| debug_on_failure = | ||
| (*options)["debug_on_failure"] | ||
| .doc("Run an aditional rhs if the solver fails with extra tracking") | ||
| .withDefault(false); |
There was a problem hiding this comment.
It is now disabled by default, and needs to be enabled with solver:debug_on_failure=true
|
Ok, I think we understand this a bit better now. I'd still be much happier if this was a macro and only turned on for |
That is a fair point. I do understand that worry, but at the same time I am convinced it is orthogonal to |
Useful for physics module to record additional data if the solver is failing
| Options* Field3D::track(const BoutReal& change, std::string operation) { | ||
| if (tracking and tracking_state) { | ||
| const std::string outname{fmt::format("track_{:s}_{:d}", selfname, tracking_state++)}; | ||
| tracking->set(outname, change, "tracking"); |
There was a problem hiding this comment.
warning: implicit conversion 'Options *' -> bool [readability-implicit-bool-conversion]
| tracking->set(outname, change, "tracking"); | |
| if ((tracking != nullptr) and tracking_state) { |
| Options* Field3D::track(const BoutReal& change, std::string operation) { | ||
| if (tracking and tracking_state) { | ||
| const std::string outname{fmt::format("track_{:s}_{:d}", selfname, tracking_state++)}; | ||
| tracking->set(outname, change, "tracking"); |
There was a problem hiding this comment.
warning: implicit conversion 'int' -> bool [readability-implicit-bool-conversion]
| tracking->set(outname, change, "tracking"); | |
| if (tracking and (tracking_state != 0)) { |
| Options& debug = *debug_ptr; | ||
| Mesh* mesh{nullptr}; | ||
| for (auto& f : f3d) { | ||
| saveParallel(debug, f.name, *f.var); |
There was a problem hiding this comment.
warning: use of undeclared identifier 'saveParallel' [clang-diagnostic-error]
saveParallel(debug, f.name, *f.var);
^| @@ -293,6 +358,57 @@ BoutReal PvodeSolver::run(BoutReal tout) { | |||
| // Check return flag | |||
| if (flag != SUCCESS) { | |||
| output_error.write("ERROR CVODE step failed, flag = {:d}\n", flag); | |||
| if (debug_on_failure) { | |||
| CVodeMemRec* cv_mem = (CVodeMem)cvode_mem; | |||
There was a problem hiding this comment.
warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
CVodeMemRec* cv_mem = (CVodeMem)cvode_mem;
^| for (auto& f : f3d) { | ||
| debug[f.name] = *f.var; | ||
| if (f.var->hasParallelSlices()) { | ||
| saveParallel(debug, f.name, *f.var); |
There was a problem hiding this comment.
warning: use of undeclared identifier 'saveParallel' [clang-diagnostic-error]
saveParallel(debug, f.name, *f.var);
^Just dumping the parallel slices does in general not work, as then collect discards that, especially if NYPE==ny
| @@ -949,6 +949,9 @@ Tensor<BoutReal> Options::as<Tensor<BoutReal>>(const Tensor<BoutReal>& similar_t | |||
| /// Convert \p value to string | |||
| std::string toString(const Options& value); | |||
|
|
|||
| /// Save the parallel fields | |||
| void saveParallel(Options& opt, const std::string name, const Field3D& tosave); | |||
There was a problem hiding this comment.
warning: parameter 'name' is const-qualified in the function declaration; const-qualification of parameters only has an effect in function definitions [readability-avoid-const-params-in-decls]
| void saveParallel(Options& opt, const std::string name, const Field3D& tosave); | |
| void saveParallel(Options& opt, std::string name, const Field3D& tosave); |
| @@ -337,6 +337,22 @@ void Options::assign<>(Tensor<BoutReal> val, std::string source) { | |||
| _set_no_check(std::move(val), std::move(source)); | |||
| } | |||
|
|
|||
| void saveParallel(Options& opt, const std::string name, const Field3D& tosave) { | |||
There was a problem hiding this comment.
warning: the const qualified parameter 'name' is copied for each invocation; consider making it a reference [performance-unnecessary-value-param]
| void saveParallel(Options& opt, const std::string name, const Field3D& tosave) { | |
| void saveParallel(Options& opt, const std::string& name, const Field3D& tosave) { |
include/bout/options.hxx:952:
- void saveParallel(Options& opt, const std::string name, const Field3D& tosave);
+ void saveParallel(Options& opt, const std::string& name, const Field3D& tosave);
|
@ZedThree anything I can do to move this forward? |
There was a problem hiding this comment.
I'm still a bit wary of this because it feels like this whole other system that needs to be everywhere for it to be useful, and the current implementation is a nice demonstration of it for a single, limited purpose.
If we're going to have it and start using it, then we definitely need some docs in the debugging section, more docstrings in the code, a bit more polish on the API, and some tests.
I'd also still really like to see some proper benchmarks comparing next and this branch.
|
|
||
| int tracking_state{0}; | ||
| Options* tracking{nullptr}; | ||
| std::string selfname; |
There was a problem hiding this comment.
Do we need this if we already have name?
| Options* track(const T& change, std::string operation); | ||
| Options* track(const BoutReal& change, std::string operation); |
There was a problem hiding this comment.
The return value seems to never be used -- just return void?
| int tracking_state{0}; | ||
| Options* tracking{nullptr}; | ||
| std::string selfname; | ||
| template <class T> |
There was a problem hiding this comment.
I know we only provide implementations for some T, but this could also have EnableIfField<T> to completely remove it as an option, rather than having a link-time error?
Actually, does this even need to be a template? Could just take const Field&?
| @@ -265,6 +266,7 @@ Field3D& Field3D::operator=(const Field3D& rhs) { | |||
|
|
|||
| Field3D& Field3D::operator=(Field3D&& rhs) { | |||
| TRACE("Field3D: Assignment from Field3D"); | |||
| track(rhs, "operator="); | |||
There was a problem hiding this comment.
I really think this should be a macro that is only enabled at higher CHECK
| @@ -683,4 +683,12 @@ inline T floor(const T& var, BoutReal f, const std::string& rgn = "RGN_ALL") { | |||
|
|
|||
| #undef FIELD_FUNC | |||
|
|
|||
| template <typename T, typename = bout::utils::EnableIfField<T>, class... Types> | |||
| inline T setName(T&& f, const std::string& name, Types... args) { | |||
There was a problem hiding this comment.
To make this a member function, make it virtual:
field.hxx:
virtual Field& setName(std::string name) {
self->name = std::move(name);
return *self;
}field2d.hxx:
Field2D& setName(std::string name) override {
Field::setName(name);
return *self;
}Formatting will have to be done at the calling site, but I think that's fine
| @@ -949,6 +949,9 @@ Tensor<BoutReal> Options::as<Tensor<BoutReal>>(const Tensor<BoutReal>& similar_t | |||
| /// Convert \p value to string | |||
| std::string toString(const Options& value); | |||
|
|
|||
| /// Save the parallel fields | |||
| void saveParallel(Options& opt, const std::string name, const Field3D& tosave); | |||
There was a problem hiding this comment.
Could this be a method rather than a free function? Then it can be called like options[name].assignParallel(value)
There was a problem hiding this comment.
I do not think that works, as we assign to name as well as f"{name}_y+1" etc ...
| std::vector<bool>::const_iterator evolve_bndry = evolve_bndrys.begin(); | ||
| for (std::vector<Field3D>::iterator ff = ffs.begin(); ff != ffs.end(); ++ff) { |
There was a problem hiding this comment.
Why not this?
| std::vector<bool>::const_iterator evolve_bndry = evolve_bndrys.begin(); | |
| for (std::vector<Field3D>::iterator ff = ffs.begin(); ff != ffs.end(); ++ff) { | |
| auto evolve_bndry = evolve_bndrys.cbegin(); | |
| for (auto& ff : ffs) { |
| run_rhs(simtime); | ||
|
|
||
| for (auto& f : f3d) { | ||
| debug[f.name] = *f.var; |
There was a problem hiding this comment.
saveParallel already includes this
| } else { | ||
| output_warn.write("debug_on_failure is currently only supported for Field3Ds"); | ||
| } |
There was a problem hiding this comment.
Invert the conditional and move this first, so we can immediately return and then unindent the rest of the block
| @@ -514,6 +527,13 @@ private: | |||
|
|
|||
| /// RegionID over which the field is valid | |||
| std::optional<size_t> regionID; | |||
|
|
|||
| int tracking_state{0}; | |||
| Options* tracking{nullptr}; | |||
There was a problem hiding this comment.
I really don't like storing raw pointers. Probably this makes most sense as weak_ptr, a non-owning reference to a shared_ptr allocated and owned elsewhere. That way it's very obvious we don't own it and we can rely on it living long enough
|
Exposing the additional PVODE options is just obvious and should probably just be it's own PR as well |
|
@ZedThree just wanted to chime in and say that having this capability available would be extremely useful for me in optimising Hermes-3. I had already played with it quite a bit, but unfortunately I had to put it on the back burner for now because of the DLS paper and other work. I fully intend to do proper testing of this to optimise Hermes-3 performance sometime in Jan-Feb, or earlier if Michael Hardman can have a go. I understand that there could be performance concerns - just wanted to say that this is much more than a small debugging benefit for me. |
