-
Notifications
You must be signed in to change notification settings - Fork 1
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
Typed Symbols #8
Conversation
} | ||
|
||
macro_rules! impl_new_builder { | ||
($($num:expr, $( ($key:ident, $key_type:ident, $var:ident) ),*);* $(;)?) => {$( |
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 is a really interesting macro... Not a blocker, but if there is any way to not do this using macros, that might be better
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.
You can do it without macros, it's just a good amount of copying and pasting. I also prefer the macros, as if we decide to support factors with > 6 variables, it's a one-liner to add in support.
I would've preferred a single new
function as well instead of new1
, new2
, etc, but couldn't figure out a way to do it, mainly since each of our keys might have a different type now
keys: Vec<Key>, | ||
residual: Box<dyn ResidualSafe>, | ||
noise: Option<Box<dyn NoiseModelSafe>>, | ||
robust: Option<Box<dyn RobustCostSafe>>, |
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.
Any way to not make this dynamic? i.e. do it at compile time
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.
We could probably make it done at compile time, but I'm not sure if there's a huge advantage? As it'll end up boxing them to send to Factor
when building anyways.
At one point I tried to not box anything, but it got hairy since that mention Factor
was generic over the residual, noise, and robust kernel. It made storing them all in Graph
nigh-impossible
/// Add a noise model to the factor. | ||
pub fn noise<N>(mut self, noise: N) -> Self | ||
where | ||
N: 'static + NoiseModel<Dim = Const<DIM_OUT>> + NoiseModelSafe, |
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.
Slick way to check at compile time that the dimension of the noise is correct...I wish GTSAM did this. Nice job!
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.
Yeah, I was pretty happy with that one :) one of my least favorite gtsam experiences it hitting a weird branch in the code and crashing due to noise model sizes
If you have any other gtsam issues you think could be solved at compile time, let me know and we can investigate
@@ -11,12 +11,12 @@ use crate::{ | |||
/// consists of the relevant keys, a [MatrixBlock] A, and a [VectorX] b. Again, | |||
/// this *shouldn't* ever need to be used by hand. | |||
pub struct LinearFactor { |
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.
If it doesn't need to be used by hand, maybe consider making this not public?
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.
That'd probably be a good idea. Currently pretty much everything is public, and there's a lot of the low level interface that shouldn't be. I haven't taken the time to think about it thoroughly.
@taylorpool Did you have anything you'd like to see changed on this? Or am I good to merge? |
This PR enables what I'm calling "typed symbols". In a factor graph, each character (ie
X
orL
) will always be of a single type - X is often SE(3) and L is generally in R^3. This feature enforces this constraint at compile time to disallow any type mismatches. This reduces a number of error checking that needs to be done in theValues
container and prevents mismatching factor keys from happening.As part of this, I've introduced a new
FactorBuilder
class to help facilitate building factors.When using gtsam it's possible to mis-order the keys when creating a factor. This is now mostly impossible in fact.rs. For example, if proper construction of a stereo factor is,
then this next line will now fail to compile,
I see this as being extra helpful once IMU preintegration is added to keep all of it's keys straight.
@taylorpool Currently assigning types is done by the user via a macro, and I could use some help settling on the syntax. I currently have the first one, but have considered all of the following,
Just an FYI, rust also allows all these to be written like,
I preferred the first one to be similar to python dictionaries, but I'm not quite settled on it yet. We could also switch the semicolons to commas as well. Do you have any thoughts on the syntax? Let me know what you think, I could use an extra opinion on this.