search

borglab / GTDynamics

Full kinodynamics constraints for arbitrary robot configurations with factor graphs.
BSD 2-Clause "Simplified" License
39 stars 10 forks source link

Refactor factor constructor arguments #290

Closed gchenfc closed 10 months ago

gchenfc commented 2 years ago

This is basically a continuation of #217

Previously, factors took raw keys as arguments. However, the correct keys can be extracted from the joint object passed in. I believe allowing these raw key constructors is unsafe and this PR is being a little bit Nazi-like in banning this construction. For example,

// TwistFactor factor(example::twist_p_key, example::twist_c_key, example::qKey,
//                    example::qVelKey, example::cost_model, joint);
TwistFactor factor(example::cost_model, joint, 777);

I propose we ban the first (commented out) version to force users to use the proper DynamicsSymbol system.

I also took out the private constructors since I don't really see what utility they offer besides just making the reader jump back and forth more, but I can add them back if desired.

I know @varunagrawal took issue with removing the old constructors in #286, opting instead to keep the old constructors in addition to the new constructors, but I believe (1) this is more thematic with #217 and (2) explicitly disallowing the "raw key" versions should make things safer and less bug-prone.

varunagrawal commented 2 years ago

Simple reason why I disapprove: I need to be able to mix GTSAM and GTD keys. The IMU factors need GTSAM keys and they don't have time information built into them. :)

varunagrawal commented 2 years ago

Yeah this PR is closing off GTD to integration with other libraries. Not a good idea :(

dellaert commented 2 years ago

Yeah this PR is closing off GTD to integration with other libraries. Not a good idea :(

Agreed for now. Please let’s discuss it in our meeting before doing anything about this. @gchenfc Please stick with the PR that we discussed without adding other elements into it?

dellaert commented 2 years ago

Yeah this PR is closing off GTD to integration with other libraries. Not a good idea :(

Agreed for an hour. Please let’s discuss it in our meeting before doing anything about us. @gchenfc Please stick with the PR that we discussed without adding other elements into it?

By the way, I am not saying this is a bad idea, I’d like to learn more about the whole discussion before Doing anything. I can provide Context with respect to GTSAM and help evaluate @varunagrawal ’s concerns.

dellaert commented 2 years ago

Yeah this PR is closing off GTD to integration with other libraries. Not a good idea :(

Agreed for an hour. Please let’s discuss it in our meeting before doing anything about us. @gchenfc Please stick with the PR that we discussed without adding other elements into it?

By the way, I am not saying this is a bad idea, I’d like to learn more about the whole discussion before Doing anything. I can provide Context with respect to GTSAM and help evaluate @varunagrawal ’s concerns.

PS PS should be “agreed for now” :-)

gchenfc commented 2 years ago

Yes we can discuss this later.

I just created this PR because I was halfway through a big rebase off #286 separating out changes that weren't relevant to that PR so I decided to finish that rebase and create this PR with it.

Then basing #291 off this branch was a mistake but I can fix that later.

varunagrawal commented 10 months ago

I merged master and looks like all the changes here have already been pushed into the master branch. Hence closing.