search

cmower / optas

OpTaS: An optimization-based task specification library for trajectory optimization and model predictive control.
https://cmower.github.io/optas/
Other
101 stars 14 forks source link

Why change of names in the get_ functions to include link #71

Closed joaomoura24 closed 1 year ago

joaomoura24 commented 1 year ago

Why did you change the get_ naming convention to include link in the name?

https://github.com/cmower/optas/blob/937db570af15999439073e007bbffef558d2178b/optas/models.py#L951

I would say that it's kind of a unnecessary repetition, since all those functions require link as an input.

cmower commented 1 year ago

Before, there existed a group of functions with link, e.g. get_global_link_position, and another group without, e.g. get_global_linear_jacobian. This inconsistency shouldn't be in the library, and so the choice was to include or exclude link in the interface names. I went with keeping link in the interface name because, whilst it makes the name longer, I find it to be more readable. This shouldn't break your code since I added the deprecation warning. But it would be good for you to move towards the new method interface names so we can remove the deprecation warnings in a first release.

Actually, I'm thinking that we should change the input variable names too. link is ambiguous, it should be something like link_name instead. Especially that we have methods in RobotModel like

https://github.com/cmower/optas/blob/937db570af15999439073e007bbffef558d2178b/optas/models.py#L372

In this case, link is an object containing all the data about a link (not just the name).

joaomoura24 commented 1 year ago

OK, but we should having these discussions over issues or pull requests, otherwise the interface is changing and I am not even realising it and now there starts to be quite a few places where we are using optas. About the change of input name, it will likely break a lot of the current implementation, so we need to coordinate that - i'lll need to be changing a few packages we have on our side that now depend on optas.

cmower commented 1 year ago

OK, but we should having these discussions over issues or pull requests, otherwise the interface is changing and I am not even realising it and now there starts to be quite a few places where we are using optas.

No problem. However, I didn't want to break your code, hence adding the deprecation warning instead. But I will keep you more up-to-date with plans for changes before they happen (just in case).

About the change of input name, it will likely break a lot of the current implementation, so we need to coordinate that - i'lll need to be changing a few packages we have on our side that now depend on optas.

As above, I don't think this should break anything but happy to coordinate that transition.

cmower commented 1 year ago

I think this should be good to close?

joaomoura24 commented 1 year ago

Yes.