Joint State and Jacobian improvements
domire8 opened this issue · 9 comments
A few comments on the JointState refactoring PR have been left unresolved for a future PR.
If you are happy with the way it has been handled in #65 I can make similar modifications for JointState
@buschbapti I think we are almost there. One thing that still confuses me is why the Jacobian class implements set_rows and set_cols without changing the data variable and a set_data function without changing the rows and cols variables. Shouldn't that all happen together, i.e. when you change the data, the number of rows and cols are changed and when you change the size, the data is changed too?
For the JointState this problem is solved, as each time you call set_names, the JointState is initialized again.
Well to be honest I am not even sure what should be the usage of the rows and cols setters. At least in the public API. I would say that those should either do as you suggest or be completely removed. Regarding set_names in JointState didn't you had a problem there that is is changing the size? I believe I have modified this behavior in Jacobian but not JointState.
I agree about the rows and cols setters, we could probably remove those and in the data setter we only accept matrices of the correct size..?
I don't recall the problem you are mentioning with the JointState, from the code I can tell that initialize is called when you set the names and that makes sense in my opinion.
Well I guess this is part of the bigger improvement of making states less mutable. I am not sure if it should accept the correct size or at least the possibility to also set the transpose maybe.
Okay so would you leave this issue open? Because I was just checking in if this is something that could be closed quickly
no let's keep it open for now