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 39 commits into
base: next
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
39 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
291e4af
Merge branch 'next' of https://github.com/boutproject/BOUT-dev into p…
dschwoerer Jan 16, 2025
42fc8ff
prefer const ref
dschwoerer Jan 16, 2025
c9c13d4
Update for move of bout/version.hxx
dschwoerer Jan 16, 2025
ff93406
Prefer auto
dschwoerer Jan 16, 2025
a096cc5
Avoid warning for 3d metrics
dschwoerer Jan 16, 2025
27db46d
Apply recommendations from code review
dschwoerer Jan 16, 2025
24610c3
Explain purpose of variables
dschwoerer Jan 16, 2025
f6db2df
Apply clang-format changes
dschwoerer Jan 16, 2025
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
8 changes: 8 additions & 0 deletions include/bout/field.hxx
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
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

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 would really love this to be not be done on the caller side, because then the formatting can be easily disabled, and the overhead disappears. If that is not done, in order to avoid the overhead of name tracking, all the calls need to be done using #if guards.

from https://en.cppreference.com/w/cpp/language/member_template

A member function template cannot be virtual, and a member function template in a derived class cannot override a virtual member function from the base class.

So I guess we would be stuck with not having the member function on the base class, but only on the derived class.

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

#endif /* FIELD_H */
11 changes: 11 additions & 0 deletions include/bout/field3d.hxx
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,10 @@ 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);

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

Expand Down Expand Up @@ -514,6 +518,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?

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 have added

  /// counter for tracking, to assign unique names to the variable names

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea 👍

std::string selfname{""};
dschwoerer marked this conversation as resolved.
Show resolved Hide resolved
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&?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, does this even need to be a template? Could just take const Field&?

Yes. Otherwise it tries to create a Field - which is virtual.

I have added EnableIfField 👍

Options* track(const T& change, std::string op);
dschwoerer marked this conversation as resolved.
Show resolved Hide resolved
Options* track(const BoutReal& change, std::string op);
dschwoerer marked this conversation as resolved.
Show resolved Hide resolved
};

// Non-member overloaded operators
Expand Down
44 changes: 44 additions & 0 deletions src/field/field3d.cxx
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,7 @@ Field3D& Field3D::operator=(const Field3D& rhs) {
}

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

// Copy base slice
Field::operator=(rhs);
Expand All @@ -263,6 +264,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 @@ -283,6 +285,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 @@ -327,6 +330,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 @@ -831,3 +835,43 @@ 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 op) {
dschwoerer marked this conversation as resolved.
Show resolved Hide resolved
if (tracking and tracking_state) {
const std::string outname{fmt::format("track_{:s}_{:d}", selfname, tracking_state++)};
tracking->set(outname, change, "tracking");
(*tracking)[outname].setAttributes({
{"operation", op},
#if BOUT_USE_TRACK
{"rhs.name", change.name},
#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 op) {
dschwoerer marked this conversation as resolved.
Show resolved Hide resolved
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");
dschwoerer marked this conversation as resolved.
Show resolved Hide resolved
dschwoerer marked this conversation as resolved.
Show resolved Hide resolved
(*tracking)[outname].setAttributes({
{"operation", op},
{"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