-
Notifications
You must be signed in to change notification settings - Fork 69
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
Optimizations for DiscreteTrajectory #2981
Optimizations for DiscreteTrajectory #2981
Conversation
DiscreteTrajectory iteration benchmarks.
when unneccessary.
[Automated message from GitHub Pull Request Builder] Answer "ok to test" to trigger testing of this PR. |
ok to test |
Jenkins says:
|
The conversion is now done by a static method instead of a constructor. There is also some documentation now. Also fixed inadvertently broken forkable_test.
ok to test |
Any information on the latest error? I don’t see any errors with clang. |
It's just lint now:
|
ok to test |
physics/discrete_trajectory.hpp
Outdated
@@ -50,6 +50,9 @@ class DiscreteTrajectoryIterator | |||
DegreesOfFreedom<Frame> const& degrees_of_freedom; | |||
}; | |||
|
|||
static reference MakeReference( | |||
typename DiscreteTrajectoryTraits<Frame>::TimelineConstIterator it); |
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.
This factory is a bit tasteless. I would say that the initializer lists {it->first, it->second}
that we had previously was perfectly readable (if slightly verbose) and did not clutter the API like this function does.
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.
{it->first, it->second}
does not work for FakeTrajectoryIterator::reference
(a.k.a. Instant const&
) in forkable_test.cc
. How do you recommend I proceed?
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.
Ah, that's annoying. Do you understand why Visual Studio was complaining about the constructor? That seemed like a better approach than the factory.
If the constructor that takes a TimelineConstIterator
doesn't work, would a constructor that takes a Timeline::value_type
(i.e., the pair itself) by reference work?
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 suspect it is because I forgot about FakeTrajectory
and didn't handle it at all until 89ca90a.
I'm trying the constructor approach again but this time replacing FakeTrajectoryIterator::reference
with a wrapper struct with the proper constructor. Hopefully Visual Studio is happy this time.
Some more lint:
|
This looks fine now, other than one remaining lint error:
|
Thanks for your contribution. Merging. |
In particular,
DiscreteTrajectory::EvaluatePosition
and friends now only compute the interpolation when necessary (as suggested by @pleroy in Downsampling is slow #2961).Forkable::{begin, front, back}
have been made more efficient.Accordingly, DiscreteTrajectory benchmarks for
t_max
,t_min
, andEvaluateDegreesOfFreedom
have been added. Additionally, some unrelated DiscreteTrajectory benchmarks have been improved:find
andlower_bound
benchmarks now correctly adapt to trajectory size.begin
andend
.Here are the relevant benchmark diffs from fc81d60 to f52362a: