Allow descriptions of output variables; save some diagnostics for solvers #2064

johnomotani · 2020-08-06T18:16:29Z

Descriptions of output variables

Adds an optional argument to Datafile::add(), Solver::add() and bout_solve() which can be passed a description as a std::string. The description is then added to the output files as an attribute of the variable. This should be useful for making the output files self-documenting. This PR adds the mechanism, but variables written by BOUT++ still need a description adding in a future PR (apart from the new solver diagnostic variables described below).

Save some diagnostics for solvers

When solver:diagnose=true is set, CVODE and ARKODE print out some useful information. This PR saves those numbers to the dump files to make them easier to access in post-processing.

The diagnostics are always saved, regardless of solver:diagnose setting. The setting is useful to avoid cluttering up stdout, but I don't see any benefit to not saving into dump files - the diagnostics are scalars so don't take up much space, and it's nice to be able to go back and look at them without having decided to set the setting before the run.

The diagnostic variables have descriptions copied from the CVODE and ARKODE manuals which are written as attributes using the new description argument described above. These help make the output files a little less cryptic, since SUNDIALS variable names are not very descriptive (e.g. nliters).

add documentation of the 'description' arguments of Datafile::add() and Solver::add() to the manual.

Allows output dump files to be more self-documenting.

Cannot just call setAttribute() in Solver::outputVars() because DataFile might not be open at that point. Instead need to pass through and set at the same point that the Field attributes are written.

johnomotani · 2020-08-06T18:39:06Z

This PR implements descriptions for output variables, which would be useful for #2065, because I wanted to add descriptions of the diagnostic variables here. The description-handling parts (09b5fa2 and 6fbf9cb) could be split into a separate PR easily if that helps.

BTW the way this is implemented means PhysicsModels can also add descriptions, with the new, optional 'description' arguments to Datafile::add() and Solver::add(). Should probably add some description of that in the manual, but let's agree on the interface design first...

Datafile::setAttribute() opens the DataFormat object and then calls its setAttribute() method. Don't want to do this inside Datafile::write() where the DataFormat is already opened.

Poiner to the int or BoutReal is stored in a VarStr, so need to pass by reference and not by value, otherwise the pointer ends up pointing to some random place.

johnomotani · 2020-08-07T15:10:30Z

I think I fixed the memory errors. Had accidentally passed int or BoutReal by value before storing a pointer to the address.

If solver type is changed, diagnostic variables will change and so might cause errors if they are added to restart files but cannot be read. So do not add diagnostic variables to the restart files.

Previously used '0' and '1' which are correct but less explicit.

Pass description through to solver->add()

johnomotani · 2020-08-21T14:32:09Z

I've added the description argument to the manual. Noticed that we recommend using bout_solve() there rather than solver->add() so added the description argument to bout_solve() as well.

This PR is ready for review now.

github-actions

clang-tidy made some suggestions

github-actions · 2020-12-10T21:03:40Z

include/bout/solver.hxx

@@ -409,6 +410,24 @@ protected:
  std::vector<VarStr<Vector2D>> v2d;
  std::vector<VarStr<Vector3D>> v3d;

+  /// Vectors of diagnostic variables to save
+  std::vector<VarStr<int>> diagnostic_int;


warning: member variable diagnostic_int has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]

std::vector<VarStr<int>> diagnostic_int; ^

github-actions · 2020-12-10T21:03:41Z

include/bout/solver.hxx

@@ -409,6 +410,24 @@ protected:
  std::vector<VarStr<Vector2D>> v2d;
  std::vector<VarStr<Vector3D>> v3d;

+  /// Vectors of diagnostic variables to save
+  std::vector<VarStr<int>> diagnostic_int;


warning: member variable diagnostic_int has protected visibility [misc-non-private-member-variables-in-classes]

github-actions · 2020-12-10T21:03:41Z

include/bout/solver.hxx

@@ -409,6 +410,24 @@ protected:
  std::vector<VarStr<Vector2D>> v2d;
  std::vector<VarStr<Vector3D>> v3d;

+  /// Vectors of diagnostic variables to save
+  std::vector<VarStr<int>> diagnostic_int;
+  std::vector<VarStr<BoutReal>> diagnostic_BoutReal;


warning: member variable diagnostic_BoutReal has protected visibility [cppcoreguidelines-non-private-member-variables-in-classes]

std::vector<VarStr<BoutReal>> diagnostic_BoutReal; ^

github-actions · 2020-12-10T21:03:41Z

include/bout/solver.hxx

@@ -409,6 +410,24 @@ protected:
  std::vector<VarStr<Vector2D>> v2d;
  std::vector<VarStr<Vector3D>> v3d;

+  /// Vectors of diagnostic variables to save
+  std::vector<VarStr<int>> diagnostic_int;
+  std::vector<VarStr<BoutReal>> diagnostic_BoutReal;


warning: member variable diagnostic_BoutReal has protected visibility [misc-non-private-member-variables-in-classes]

include/bout/solver.hxx

github-actions · 2020-12-10T21:03:42Z

src/solver/impls/arkode/arkode.cxx

@@ -506,15 +518,22 @@ int ArkodeSolver::run() {
      throw BoutException("ARKode timestep failed\n");
    }

+    // Get additional diagnostics
+    long int temp_long_int, temp_long_int2;


warning: multiple declarations in a single statement reduces readability [readability-isolate-declaration]

long int temp_long_int, temp_long_int2; ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

github-actions · 2020-12-10T21:03:42Z

src/solver/impls/arkode/arkode.cxx

@@ -506,15 +518,22 @@ int ArkodeSolver::run() {
      throw BoutException("ARKode timestep failed\n");
    }

+    // Get additional diagnostics
+    long int temp_long_int, temp_long_int2;


warning: variable temp_long_int is not initialized [cppcoreguidelines-init-variables]

long int temp_long_int, temp_long_int2; ^ = 0

github-actions · 2020-12-10T21:03:42Z

src/solver/impls/arkode/arkode.cxx

@@ -506,15 +518,22 @@ int ArkodeSolver::run() {
      throw BoutException("ARKode timestep failed\n");
    }

+    // Get additional diagnostics
+    long int temp_long_int, temp_long_int2;


warning: variable temp_long_int2 is not initialized [cppcoreguidelines-init-variables]

long int temp_long_int, temp_long_int2; ^ = 0

github-actions · 2020-12-10T21:03:42Z

src/solver/impls/cvode/cvode.cxx

@@ -385,16 +404,39 @@ int CvodeSolver::run() {
      throw BoutException("SUNDIALS CVODE timestep failed\n");
    }

+    // Get additional diagnostics
+    long int temp_long_int;


warning: variable temp_long_int is not initialized [cppcoreguidelines-init-variables]

long int temp_long_int; ^ = 0

johnomotani · 2020-12-10T23:58:31Z

What's the clang-tidy bot supposed to do if you 'fix' the things it suggests? It doesn't seem to have removed the comments like I'd hope. The changes I made weren't directly what it suggested though (I passed by reference instead of using a std::move), so that might be why?

ZedThree · 2020-12-11T08:33:49Z

No, I'm afraid it's not as clever as that. For now you can just click "resolve" on the comment. I can probably make it do that automatically if there's an API for it though

…

On Thu, 10 Dec 2020, 23:58 johnomotani, ***@***.***> wrote: What's the clang-tidy bot supposed to do if you 'fix' the things it suggests? It doesn't seem to have removed the comments like I'd hope. The changes I made weren't directly what it suggested though (I passed by reference instead of using a std::move), so that might be why? — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#2064 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AALLAXTYGC7S3WLU5NNBVIDSUFODLANCNFSM4PW3C37A> .

johnomotani · 2020-12-11T08:44:12Z

No, I'm afraid it's not as clever as that. For now you can just click
"resolve" on the comment. I can probably make it do that automatically if
there's an API for it though

Thanks @ZedThree! The PEP8 bot somehow does this, but I don't know how. Would be nice, but clicking 'resolve' on addressed issues is fine with me - I was just wondering if there was a feature that wasn't working right.

ZedThree · 2020-12-11T09:04:18Z

I will investigate the PEP8 bot and steal features from it 😄

…

On Fri, 11 Dec 2020, 08:44 johnomotani, ***@***.***> wrote: No, I'm afraid it's not as clever as that. For now you can just click "resolve" on the comment. I can probably make it do that automatically if there's an API for it though Thanks @ZedThree <https://github.com/ZedThree>! The PEP8 bot somehow does this, but I don't know how. Would be nice, but clicking 'resolve' on addressed issues is fine with me - I was just wondering if there was a feature that wasn't working right. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2064 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AALLAXTAUDNCGUGUIG25KJ3SUHLWVANCNFSM4PW3C37A> .

johnomotani added 7 commits August 6, 2020 19:02

Allow saving diagnostic variables from Solver subclasses

5d14356

Save diagnostics to dump files in CvodeSolver

cc8e9b2

Save diagnostics to output files in ArkodeSolver

3ff1699

Optional arguments to give descriptions for output variables in Solver

09b5fa2

Allows output dump files to be more self-documenting.

Descriptions for diagnostic variables in CvodeSolver

734782b

Descriptions for diagnostic variables in ArkodeSolver

0056c4e

Pass description through DataFile::add()

6fbf9cb

Cannot just call setAttribute() in Solver::outputVars() because DataFile might not be open at that point. Instead need to pass through and set at the same point that the Field attributes are written.

johnomotani added the feature label Aug 6, 2020

johnomotani mentioned this pull request Aug 6, 2020

Self documenting output files #2065

Open

johnomotani added work in progress Not ready for merging backport candidiate Does not break backward compatibility, so can be back-ported to v4.4 labels Aug 6, 2020

johnomotani added 4 commits August 6, 2020 19:50

Add description argument in SlepcSolver overrides

0317dbb

Call file->setAttribute() not setAttribute()

7f03a34

Datafile::setAttribute() opens the DataFormat object and then calls its setAttribute() method. Don't want to do this inside Datafile::write() where the DataFormat is already opened.

Initialise diagnostic variables in CvodeSolver and ArkodeSolver

7f483b9

Pass by reference in add_diagnostic_int() and add_diagnostic_BoutReal()

3cd2695

Poiner to the int or BoutReal is stored in a VarStr, so need to pass by reference and not by value, otherwise the pointer ends up pointing to some random place.

Do not add Solver diagnostic variables to restart files

1e351f9

If solver type is changed, diagnostic variables will change and so might cause errors if they are added to restart files but cannot be read. So do not add diagnostic variables to the restart files.

johnomotani requested review from ZedThree and bendudson August 7, 2020 15:16

johnomotani added 5 commits August 18, 2020 11:51

Merge remote-tracking branch 'origin/next' into solver_diagnostics_dump

404166a

Merge remote-tracking branch 'origin/next' into solver_diagnostics_dump

9b4cc9e

Use 'false' and 'true' for save_repeat args of Datafile::add() in manual

1928c05

Previously used '0' and '1' which are correct but less explicit.

Add description argument to bout_solve() function/methods

e8c30b4

Pass description through to solver->add()

Document 'description' argument for bout_solve() and Datafile::add()

157652d

johnomotani removed the work in progress Not ready for merging label Aug 21, 2020

johnomotani mentioned this pull request Aug 21, 2020

Allow descriptions of output variables; save some diagnostics for solvers (v4.4) #2086

Merged

johnomotani added the documentation label Aug 21, 2020

johnomotani changed the title ~~Save some diagnostics for solvers~~ Allow descriptions of output variables; save some diagnostics for solvers Aug 21, 2020

Merge branch 'next' into solver_diagnostics_dump

cd7403a

github-actions bot reviewed Dec 10, 2020

View reviewed changes

Fix a couple of clang-tidy warnings

280fed5

johnomotani force-pushed the solver_diagnostics_dump branch from d6c50d2 to 280fed5 Compare December 10, 2020 22:24

bendudson approved these changes Dec 28, 2020

View reviewed changes

bendudson merged commit d473853 into next Dec 28, 2020

bendudson deleted the solver_diagnostics_dump branch December 28, 2020 10:45

johnomotani mentioned this pull request Jan 3, 2021

Write descriptions for std::vector<int> and std::string variables #2190

Merged

ZedThree mentioned this pull request Jan 4, 2021

test-invpar timing out #2185

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow descriptions of output variables; save some diagnostics for solvers #2064

Allow descriptions of output variables; save some diagnostics for solvers #2064

johnomotani commented Aug 6, 2020 •

edited

johnomotani commented Aug 6, 2020

johnomotani commented Aug 7, 2020

johnomotani commented Aug 21, 2020

github-actions bot left a comment

github-actions bot Dec 10, 2020

github-actions bot Dec 10, 2020

github-actions bot Dec 10, 2020

github-actions bot Dec 10, 2020

github-actions bot Dec 10, 2020

github-actions bot Dec 10, 2020

github-actions bot Dec 10, 2020

github-actions bot Dec 10, 2020

johnomotani commented Dec 10, 2020

ZedThree commented Dec 11, 2020 via email

johnomotani commented Dec 11, 2020

ZedThree commented Dec 11, 2020 via email

Allow descriptions of output variables; save some diagnostics for solvers #2064

Allow descriptions of output variables; save some diagnostics for solvers #2064

Conversation

johnomotani commented Aug 6, 2020 • edited

Descriptions of output variables

Save some diagnostics for solvers

johnomotani commented Aug 6, 2020

johnomotani commented Aug 7, 2020

johnomotani commented Aug 21, 2020

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot Dec 10, 2020

Choose a reason for hiding this comment

github-actions bot Dec 10, 2020

Choose a reason for hiding this comment

github-actions bot Dec 10, 2020

Choose a reason for hiding this comment

github-actions bot Dec 10, 2020

Choose a reason for hiding this comment

github-actions bot Dec 10, 2020

Choose a reason for hiding this comment

github-actions bot Dec 10, 2020

Choose a reason for hiding this comment

github-actions bot Dec 10, 2020

Choose a reason for hiding this comment

github-actions bot Dec 10, 2020

Choose a reason for hiding this comment

johnomotani commented Dec 10, 2020

ZedThree commented Dec 11, 2020 via email

johnomotani commented Dec 11, 2020

ZedThree commented Dec 11, 2020 via email

johnomotani commented Aug 6, 2020 •

edited