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

Pvcode + Cvode improvements #2889

Open
wants to merge 31 commits into
base: next
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 30 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
fee27c9
Dump field before and after rhs evaluation for debugging
dschwoerer Nov 3, 2023
96da2e9
Add setName function
dschwoerer Nov 3, 2023
e56981c
Set div_par and grad_par names
dschwoerer Jun 19, 2023
6b2c132
Dump debug file if PVODE fails
dschwoerer Jun 19, 2023
df2d661
Add tracking to Field3D
dschwoerer Jun 19, 2023
263f9fe
Add tracking to Field3D
dschwoerer Jun 19, 2023
bac4ca9
cvode: Add option to use Adams Moulton solver instead of BDF
dschwoerer Apr 25, 2023
708bdcb
Expose more pvode option to user
dschwoerer Mar 29, 2023
d88b454
Fix bad cherry-pick
dschwoerer Mar 19, 2024
023bc41
Update to new API
dschwoerer Mar 19, 2024
affc995
Fix documentation
dschwoerer Mar 19, 2024
71f5b6a
Apply clang-format changes
dschwoerer Mar 19, 2024
31fd461
Apply recomendations from code-review
dschwoerer Mar 20, 2024
17e46cf
Use more meaningful names
dschwoerer Mar 20, 2024
9c0ae16
Apply clang-format changes
dschwoerer Mar 20, 2024
4a17b49
Apply suggestions from code review
dschwoerer Mar 20, 2024
2f7c3c0
Workaround for gcc 9.4
dschwoerer Mar 20, 2024
d611758
Add option to debug on failure
dschwoerer Apr 9, 2024
db96b7e
Add option to euler solver to dump debug info
dschwoerer Apr 9, 2024
84bfcef
Disable tracking once we are done
dschwoerer Apr 15, 2024
73265df
Allow to dump every timestep with euler
dschwoerer Apr 15, 2024
8ff388a
Apply clang-format changes
dschwoerer Apr 15, 2024
6724e75
Dump also parallel fields by default
dschwoerer Aug 9, 2024
28212c2
Also dump parallel fields
dschwoerer Aug 9, 2024
9cf0fba
Clarify which div_par has been used
dschwoerer Aug 9, 2024
97b67e1
Expose tracking
dschwoerer Aug 9, 2024
77e08ec
Apply clang-format changes
dschwoerer Oct 22, 2024
ba6fc6c
Add simple interface to store parallel fields
dschwoerer Aug 9, 2024
3c0d6a8
Apply clang-format changes
dschwoerer Oct 22, 2024
8e578fc
Merge branch 'next' into pvcode-cvode-improvements
dschwoerer Oct 22, 2024
d0669cd
Do not use numberParallelSlices()
dschwoerer Oct 22, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions include/bout/field.hxx
Original file line number Diff line number Diff line change
Expand Up @@ -683,4 +683,19 @@ 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) {
Copy link
Member

Choose a reason for hiding this comment

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

I think most other setters like this are member functions

Copy link
Member

Choose a reason for hiding this comment

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

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

#if BOUT_USE_TRACK
f.name = fmt::format(name, args...);
#endif
return f;
}

#endif /* FIELD_H */
20 changes: 20 additions & 0 deletions include/bout/field3d.hxx
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,17 @@ public:
/// cuts on closed field lines?
bool requiresTwistShift(bool twist_shift_enabled);

/// Enable a special tracking mode for debugging
/// Save all changes that, are done to the field, to tracking
Field3D& enableTracking(const std::string& name, Options& tracking);

/// Disable tracking
Field3D& disableTracking() {
tracking = nullptr;
tracking_state = 0;
return *this;
}

/////////////////////////////////////////////////////////
// Data access

Expand Down Expand Up @@ -499,6 +510,8 @@ public:

int size() const override { return nx * ny * nz; };

Options* getTracking() { return tracking; };

private:
/// Array sizes (from fieldmesh). These are valid only if fieldmesh is not null
int nx{-1}, ny{-1}, nz{-1};
Expand All @@ -514,6 +527,13 @@ private:

/// RegionID over which the field is valid
std::optional<size_t> regionID;

int tracking_state{0};
Copy link
Member

Choose a reason for hiding this comment

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

What is this for?

Options* tracking{nullptr};
Copy link
Member

Choose a reason for hiding this comment

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

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

std::string selfname;
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this if we already have name?

template <class T>
Copy link
Member

Choose a reason for hiding this comment

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

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&?

Options* track(const T& change, std::string operation);
Options* track(const BoutReal& change, std::string operation);
Comment on lines +535 to +536
Copy link
Member

Choose a reason for hiding this comment

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

The return value seems to never be used -- just return void?

};

// Non-member overloaded operators
Expand Down
3 changes: 3 additions & 0 deletions include/bout/options.hxx
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

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

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]

Suggested change
void saveParallel(Options& opt, const std::string name, const Field3D& tosave);
void saveParallel(Options& opt, std::string name, const Field3D& tosave);

Copy link
Member

Choose a reason for hiding this comment

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

Could this be a method rather than a free function? Then it can be called like options[name].assignParallel(value)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not think that works, as we assign to name as well as f"{name}_y+1" etc ...


/// Output a stringified \p value to a stream
///
/// This is templated to avoid implict casting: anything is
Expand Down
2 changes: 1 addition & 1 deletion manual/sphinx/user_docs/bout_options.rst
Original file line number Diff line number Diff line change
Expand Up @@ -889,7 +889,7 @@ Fields can also be stored and written::
Options fields;
fields["f2d"] = Field2D(1.0);
fields["f3d"] = Field3D(2.0);
bout::OptionsIO::create("fields.nc").write(fields);
bout::OptionsIO::create("fields.nc")->write(fields);

This allows the input settings and evolving variables to be
combined into a single tree (see above on joining trees) and written
Expand Down
48 changes: 48 additions & 0 deletions src/field/field3d.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,7 @@ Field3D& Field3D::operator=(const Field3D& rhs) {
}

TRACE("Field3D: Assignment from Field3D");
track(rhs, "operator=");

// Copy base slice
Field::operator=(rhs);
Expand All @@ -265,6 +266,7 @@ Field3D& Field3D::operator=(const Field3D& rhs) {

Field3D& Field3D::operator=(Field3D&& rhs) {
TRACE("Field3D: Assignment from Field3D");
track(rhs, "operator=");
ZedThree marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

I really think this should be a macro that is only enabled at higher CHECK


// Move parallel slices or delete existing ones.
yup_fields = std::move(rhs.yup_fields);
Expand All @@ -285,6 +287,7 @@ Field3D& Field3D::operator=(Field3D&& rhs) {

Field3D& Field3D::operator=(const Field2D& rhs) {
TRACE("Field3D = Field2D");
track(rhs, "operator=");

/// Check that the data is allocated
ASSERT1(rhs.isAllocated());
Expand Down Expand Up @@ -329,6 +332,7 @@ void Field3D::operator=(const FieldPerp& rhs) {

Field3D& Field3D::operator=(const BoutReal val) {
TRACE("Field3D = BoutReal");
track(val, "operator=");

// Delete existing parallel slices. We don't copy parallel slices, so any
// that currently exist will be incorrect.
Expand Down Expand Up @@ -833,3 +837,47 @@ Field3D::getValidRegionWithDefault(const std::string& region_name) const {
void Field3D::setRegion(const std::string& region_name) {
regionID = fieldmesh->getRegionID(region_name);
}

Field3D& Field3D::enableTracking(const std::string& name, Options& _tracking) {
tracking = &_tracking;
tracking_state = 1;
selfname = name;
return *this;
}

template <class T>
Options* Field3D::track(const T& change, std::string operation) {
if (tracking != nullptr and tracking_state != 0) {
const std::string outname{fmt::format("track_{:s}_{:d}", selfname, tracking_state++)};
tracking->set(outname, change, "tracking");
// Workaround for bug in gcc9.4
#if BOUT_USE_TRACK
const std::string changename = change.name;
#endif
(*tracking)[outname].setAttributes({
{"operation", operation},
#if BOUT_USE_TRACK
{"rhs.name", changename},
#endif
});
return &(*tracking)[outname];
}
return nullptr;
}

template Options* Field3D::track<Field3D>(const Field3D&, std::string);
template Options* Field3D::track<Field2D>(const Field2D&, std::string);
template Options* Field3D::track<FieldPerp>(const FieldPerp&, std::string);

Options* Field3D::track(const BoutReal& change, std::string operation) {
if (tracking and tracking_state) {
dschwoerer marked this conversation as resolved.
Show resolved Hide resolved
dschwoerer marked this conversation as resolved.
Show resolved Hide resolved
const std::string outname{fmt::format("track_{:s}_{:d}", selfname, tracking_state++)};
tracking->set(outname, change, "tracking");
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: implicit conversion 'Options *' -> bool [readability-implicit-bool-conversion]

Suggested change
tracking->set(outname, change, "tracking");
if ((tracking != nullptr) and tracking_state) {

Copy link
Contributor

Choose a reason for hiding this comment

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

warning: implicit conversion 'int' -> bool [readability-implicit-bool-conversion]

Suggested change
tracking->set(outname, change, "tracking");
if (tracking and (tracking_state != 0)) {

(*tracking)[outname].setAttributes({
{"operation", operation},
{"rhs.name", "BR"},
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{"rhs.name", "BR"},
{"rhs.name", "BoutReal"},

});
return &(*tracking)[outname];
}
return nullptr;
}
14 changes: 14 additions & 0 deletions src/field/gen_fieldops.jinja
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,10 @@
}
{% endif %}

#if BOUT_USE_TRACK
{{out.name}}.name = fmt::format("{:s} {{operator}} {:s}", {{'"BR"' if lhs == "BoutReal" else lhs.name + ".name"}}
, {{'"BR"' if rhs == "BoutReal" else rhs.name + ".name"}});
#endif
checkData({{out.name}});
return {{out.name}};
}
Expand Down Expand Up @@ -129,9 +133,19 @@
}
{% endif %}

{% if lhs == "Field3D" %}
track(rhs, "operator{{operator}}=");
{% endif %}
#if BOUT_USE_TRACK
name = fmt::format("{:s} {{operator}}= {:s}", this->name, {{'"BR"' if rhs == "BoutReal" else rhs.name + ".name"}});
#endif

checkData(*this);

} else {
{% if lhs == "Field3D" %}
track(rhs, "operator{{operator}}=");
{% endif %}
(*this) = (*this) {{operator}} {{rhs.name}};
}
return *this;
Expand Down
Loading
Loading