Key Helpers return `gtsam::Key`

[varunagrawal]

Something that has been a bit irksome for me has been the need to specify .key() for the various key helpers.

Example:

j_key = gtd.JointAngleKey(0, t=10).key()

instead of

j_key = gtd.JointAngleKey(0, t=10)

which is both cleaner and shorter. If the function says "Key" it should return a Key, right?

This PR updates the code to remedy this. Updating dependent code should be a cinch (global replace .key() with the empty string) and should make the library more user friendly.

[gchenfc]

Love the thought since, as evidenced by all the .key() you had to change in my code, I also get annoyed by this (waiting 3min for an optimization to run only for something stupid to runtime error at the end during extraction because of .key). So thanks!

But I wonder if this approach is the right way to handle this, since now we lose a lot of debugging help when we print keys, since most keys now will be in native Key form instead of DynamicsSymbol. Specifically, I wonder if pybind implicitly_convertible is an option here? I vaguely recall looking into this before but can't remember what I concluded.

[varunagrawal]

For debugging in C++, it should be straightforward to convert the Key back to a DynamicsSymbol (I believe there is a dicrect constructor for that). Plus the GTDKeyFormatter already handles this well if you prefer print debugging.

I am not sure I follow your comment on implicitly_convertible. I may be missing some context?

[varunagrawal]

Also, from personal experience, I've never had to extract the keys and convert them. Rather I just use the GTDKeyFormatter and get the same debugging info. This works well since I use a mix of GTSAM and GTD keys and the key formatter converts them all to the correct GTD format. 🙂

[gchenfc]

For debugging in C++, it should be straightforward to convert the Key back to a DynamicsSymbol (I believe there is a dicrect constructor for that). Plus the GTDKeyFormatter already handles this well if you prefer print debugging.

I would argue that:
print(gtd.DynamicsSymbol(key))
or
print(gtd.GTDKeyFormatter(key))
are more annoying than
key.key()

I am not sure I follow your comment on implicitly_convertible. I may be missing some context?

https://pybind11.readthedocs.io/en/stable/advanced/classes.html#implicit-conversions

This is I think the exact use case we have, where we want Key to be implicitly convertible to DynamicsSymbol. But this may not be possible since Key is typedef'd to a native type (not pybind object). But might be worth looking into.

Also, from personal experience, I've never had to extract the keys and convert them. Rather I just use the GTDKeyFormatter and get the same debugging info. This works well since I use a mix of GTSAM and GTD keys and the key formatter converts them all to the correct GTD format. 🙂

From my personal experience, I have ;) and do use the debugging info enough that I prefer not to have to type GTDKeyFormatter if possible ;)

More generally speaking, this PR is "erasing type information" which I don't think is a great design choice?

---

My personal opinion: if we can get implicitly_convertible working, we should do that. Otherwise, I think the annoyance of typing .key() is worth having normal DynamicsSymbol usage. Or perhaps an alternative: rename JointAngleKey to JointAngleSymbol or something, and have a separate JointAngleKey that returns the .key() version (so basically what you did except have another XXXSymbol version that still returns DynamicsSymbol). But I'm not sure how I feel about that either.

[gchenfc]

FYI https://github.com/pybind/pybind11/pull/264

[varunagrawal]

I don't understand either of the asks.

The implicit conversion is already happening at the cpp level so why do we need to do it at the python wrapper level again?

Also, I don't believe you have to call DynamicsSymbol on any key. Rather you should be able to pass DynamicsSymbol as a key directly or retrieve a key as DynamicsSymbol and the compiler will take care of the necessary conversion.

This was the primary reason for this PR. When the conversion is implicitly happening, there should be no reason for us to call .key separately.

[gchenfc]

Varun and I chatted offline - here's the summary/conclusion:

The closest analog to these JointAngleKey, JointVelKey, ... functions is gtsam::symbol_shorthand, and those return Keys, i.e. inline Key X(int i) { return Symbol('X', i); }, so it makes the most sense for these functions to follow the same pattern. Therefore this PR makes sense. i.e.

inline gtsam::Key JointAngleKey(args) { return DynamicsSymbol(args); }

Varun will make a small commit and I'll review shortly thereafter.