The downsampling algorithm #1617
Conversation
| const_iterator cbegin() const; | ||
| const_iterator cend() const; | ||
|
|
||
| size_type size() const; |
There was a problem hiding this comment.
It is O(N), but it can be Θ(1), so this would be misleading.
| // (*itᵢ₋₁, *itᵢ) fits |samples| within |tolerance|, but the interpolation | ||
| // of (*itᵢ₋₁, *(it + 1)ᵢ) does not, i.e., the iterators delimit "maximal" | ||
| // fitting polynomials. | ||
| // Note that it follows from the above that itᵣ < samples.end() - 1, so that at |
There was a problem hiding this comment.
This sentence doesn't really parse.
numerics/fit_hermite_spline.hpp
Outdated
| namespace internal_fit_hermite_spline { | ||
|
|
||
| // Given |samples| for which the arguments, values, and derivatives can be | ||
| // obtained via the given functors, returns a sequence of iterators |
There was a problem hiding this comment.
You need to document the functors better, they are totally mysterious in the template declarations below.
In fact it would be much more readable to do something like, e.g.:
typename Argument,
typename Value,
std::function<Argument(typename Samples::iterator)> const& get_argument,
... // Same for get_value.
std::function<Derivative<Value>(typename Sample::iterator)> const& get_derivative,
As written, all the contract is hidden in the implementation.
There was a problem hiding this comment.
numerics/fit_hermite_spline_body.hpp
Outdated
| return {}; | ||
| } | ||
|
|
||
| auto interpolation_error = [get_argument, get_value, get_derivative]( |
| // Invariant: The Hermite interpolant on [samples.begin(), lower] is below | ||
| // the tolerance, the Hermite interpolant on [samples.begin(), upper] is | ||
| // above. | ||
| auto lower = samples.begin() + 1; |
There was a problem hiding this comment.
Could this be implemented using std::binary_search?
There was a problem hiding this comment.
std::lower_bound and std::binary_search require (25.4.3.1:1, 25.4.3.4:1)
The elements
eof[first,last)shall be partitioned with respect to the expressione < valueorcomp(e, value).
and std::upper_bound requires (25.4.3.2:1)
The elements
eof[first,last)shall be partitioned with respect to the expression!(value < e)or!comp(value, e).
partitioned with respect to an expression is defined as (25.4:6)
A sequence
[start,finish)is partitioned with respect to an expressionf(e)if there exists an integernsuch that for all0 <= i < distance(start, finish),f(*(start + i))is true if and only ifi < n.
Another option, std::partition_point, requires (25.3.13:16)
[first,last)shall be partitioned bypred, i.e. all elements that satisfypredshall appear before those that do not.
This is not satisfied by the range [samples.begin()+1, samples.end()[, which may have several alternations of the predicate (but has at least one, and an odd number), as described in the comment.
All citations from the working draft N4296 of JTC 1/SC 22/WG 21.
There was a problem hiding this comment.
Added a proof of termination.
numerics/fit_hermite_spline_test.cpp
Outdated
|
|
||
| TEST_F(FitHermiteSplineTest, Sinusoid) { | ||
| AngularFrequency const ω = 1 * Radian / Second; | ||
| auto const f = [ω, this](Instant const t) { |
There was a problem hiding this comment.
Instant const& at several places.
numerics/fit_hermite_spline_test.cpp
Outdated
| auto const df = [ω, this](Instant const t) { | ||
| return -ω * Sin(ω *(t - t0_)) * Metre / Radian; | ||
| }; | ||
| auto t = DoublePrecision<Instant>(t0_); |
There was a problem hiding this comment.
Put this immediately before the loops.
numerics/fit_hermite_spline_test.cpp
Outdated
| // Also note that |Pointee| doesn't work with iterators, so | ||
| // |Pointee(Field(&Sample::t, _))| is not an option. | ||
| std::function<Instant(std::vector<Sample>::const_iterator)> const get_time = | ||
| [](auto it) { return it->t; }; |
eggrobin
force-pushed
the
actual-downsampling-algorithm
branch
from
ff0d73f to
8e10071
Compare
|
This is part of the work on #228. |
No description provided.