Imu Examples Refactor #872
Conversation
varunagrawal
commented
Sep 10, 2021
varunagrawal
commented
- Custom preintegration params support.
- Make plotting into its own function.
- Minor formatting.
|
Gentle reminder |
|
@johnwlambert gentle reminder |
| parser.add_argument("--twist_scenario", | ||
| default="sick_twist", | ||
| choices=("zero_twist", "forward_twist", "loop_twist", | ||
| "sick_twist")) |
There was a problem hiding this comment.
Hi Varun, maybe you could define "sick_twist" in the "help" section for this argparse argument?
There was a problem hiding this comment.
This should be documented in the ScenarioRunner file.
There was a problem hiding this comment.
Makes sense. Might be nice to include a few words about it here, just to be self-contained.
| state = self.scenario.navState(i) | ||
| graph.push_back( | ||
| gtsam.PriorFactorPose3(X(i), state.pose(), self.priorNoise)) | ||
| graph.push_back( | ||
| gtsam.PriorFactorVector(V(i), state.velocity(), self.velNoise)) | ||
|
|
||
| def optimize(self, graph, initial): | ||
| def optimize(self, graph: gtsam.NonlinearFactorGraph, | ||
| initial: gtsam.Values): |
There was a problem hiding this comment.
i'm curious how using the prior factor to a value at each time step affects performance, rather than just initializing LM with those values
There was a problem hiding this comment.
Just providing it as initial values means that there is no constraint on thayt value. The prior says that it should not deviate too much from this initial value.
There was a problem hiding this comment.
Makes sense, have you found this constraint to have a big impact on performance in your experience?
There was a problem hiding this comment.
For my use cases, yes. Something as simple as fixing the origin via a prior can be pretty powerful.
|
|
||
| print("Bias Values", result.atConstantBias(BIAS_KEY)) | ||
| print("Bias Values", values.atConstantBias(BIAS_KEY)) |
There was a problem hiding this comment.
might be nice to put in the brief docstring at the top of the file what the goal of this IMU example is and the significance of the "bias" values computed.
There was a problem hiding this comment.
This already exists right? The assumption here is that the person reading this file is already familiar with IMU Preintegration and is essentially trying to figure out how to use it.
There was a problem hiding this comment.
I see. I haven't worked with IMU pre-integration before, though, so would be beneficial for a gtsam user like me. I think there are some users that might just peruse the examples out of interest in learning more.
There was a problem hiding this comment.
For my use cases, yes. Something as simple as fixing the origin via a prior can be pretty powerful.
| plt.show() | ||
|
|
||
| def run(self, | ||
| T: int = 12, |
There was a problem hiding this comment.
might be nice in the docstring to explain what "T" represents
There was a problem hiding this comment.
Args:
T: number of seconds to perform state estimation for
compute_covariances: ...There was a problem hiding this comment.
That's the general convention where T is the total time for the trajectory. I'll add in the docstring.
There was a problem hiding this comment.
i think this one didn't get updated still
| ax = plt.gca() | ||
| ax.set_xlim3d(-self.maxDim, self.maxDim) | ||
| ax.set_ylim3d(-self.maxDim, self.maxDim) | ||
| ax.set_zlim3d(-self.maxDim, self.maxDim) | ||
|
|
||
| plt.pause(time_interval) | ||
|
|
||
| def run(self, T=12): | ||
| # simulate the loop | ||
| def run(self, T: int = 12): |
There was a problem hiding this comment.
nit: we can put a return type here:
def run(self, T: int = 12) -> NoneThere was a problem hiding this comment.
Can you do a full sweep of the type reviews rather than in bits please? It's mildly frustrating to have to make multiple of these updates especially since they are tangential to the main aspect of this PR.
There was a problem hiding this comment.
And especially since the None return type doesn't really give us any information...
There was a problem hiding this comment.
Sure, I missed this one earlier
