-
Notifications
You must be signed in to change notification settings - Fork 239
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
Add parsing of piecewise linear curves. #1149
Conversation
} | ||
|
||
return std::unique_ptr<MathLib::PiecewiseLinearInterpolation>{ | ||
new MathLib::PiecewiseLinearInterpolation{coords, values}}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could move the vectors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PiecewiseLinearInterpolation
ctors need to be modified. There are three possibilities:
- Duplicate code and replace
const&
parameters with&&
andstd::move
in initializer list. - Use universal reference, i.e.
template <typename T> PiecewiseLI(T&& coords, T&& values) : _coords(std::forward<T>(coords)), ...
. - Pass-by-value and
std::move
in initializer list.
I suggest to take the third option, because the vector's move operation is cheap and it covers the two cases of passed l-values and r-values w/o code duplication and templates.
cf. to "Effective modern c++" Item 41: "Consider pass by value for copyable parameters that are cheap to move and always copied."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fourth possibility is to implement ctor taking r-values only. Currently there are any calls with l-values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cf. to "Effective modern c++" Item 41: "Consider pass by value for copyable parameters that are cheap to move and always copied."
But we do not always copy the vectors. In fact, they are only copied in the unit tests, aren't they?
My opinion:
- IMHO the best option
- Problematic. Maybe then different calls of
std::vector
's ctor other than copy/move could be generated implicitly, e.g.,vector(std::initializer_list)
, though I'm not sure about it. Anyway in this case the avoided code duplication is neglectable. Furthermore we don't do such things in any other place, do we? - There will always be a copy and we rely on the compilers optimizer to perform the copy elision.
- There are have several other places where we've only implemented r-value reference arguments, so it wouldn't hurt (unless you can give an example that hurts 😄).
Anyway, that's only a minor issue. Don't waste time on it. If you can't decide on a specific solution, please just leave a TODO comment in the code, so we can find the spot later on. I'd vote for option 1 or 4, choose either one of them, if you like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We always copy the vectors in PiecewiseLinearInterpolation ctor. Before this PR copy is via copy, with this PR copy is via move if r-value is passed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if I get you right, "copy" is everything except storing a reference to an external vector as a data member of the class?
Still I'd like to know the opinion of the others about that "paradigm shift". Maybe I'm the only one who is concerned.
Only minor comments 👍 |
bool supp_pnts_sorted) : | ||
_supp_pnts(supporting_points), _values_at_supp_pnts(values_at_supp_pnts) | ||
PiecewiseLinearInterpolation::PiecewiseLinearInterpolation( | ||
std::vector<double> supporting_points, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like that solution, because it contradicts our current paradigm. Probably the code works: Move constructor called twice. Still, we should be extremely sure that we know by heart what we are doing here, and that it works as a general approach. It probably does, if Scott Meyers suggests it. But passing by value/ref/ref&& is already tricky enough, and this technique adds another facet. Will we remember in half a year, why we pass by value here? Can we teach others, why that's good? Will those taught be confused and then pass everything by value?
Or a different point: Do we know by heart whether void f(std::vector<int>&& v) {}
can be called with an l-value reference? And why it is like that?–You see, it's complicated 😄.
Just one comment for future development: I think we need a class for curves (or functions), which can provide the value and also possibly the tangential of the function at a given variable. |
After discussion with Tom, we move to r-values constructor and drop the const& one. |
1773862
to
fa4886e
Compare
@chleh Are your minor comments answered? If yes, from my point of view you could set the "rebase and merge" flag. |
fa4886e
to
0cfc89b
Compare
Reasoning: The vector's move operation is cheap and it covers the two cases of passed l-values and r-values w/o code duplication and templates. According to "Effective modern c++" Item 41: "Consider pass by value for copyable parameters that are cheap to move and always copied."
This way an expensive copy must be performed by the caller and is not hidden in the ctor.
The constructor performed two things simultaniously, which is not good for a ctor: Constructing the object and calculating the values at given points. If needed such code should be moved to a standalone algorithm.
0cfc89b
to
965c0b0
Compare
OpenGeoSys development has been moved to GitLab. |
Adds a
_curves
map from names toMathLib::PiecewiseLinearInterpolation
s to project data whenis present in the project file.
I currently use this feature to create time-dependent boundary conditions (but this is too early for PR); and @Yonghui56 started work on Richard's flow, where the curves are used for parameters.