lardemua / atom

Calibration tools for multi-sensor, multi-modal robotic systems
GNU General Public License v3.0
245 stars 28 forks source link

why not use the function with the same name in utilities? #942

Closed miguelriemoliveira closed 1 month ago

miguelriemoliveira commented 1 month ago

https://github.com/lardemua/atom/blob/be1aeb038fc946252ab81e9e8f68623c44aeca42/atom_calibration/scripts/automatic_data_collector#L19

miguelriemoliveira commented 1 month ago

Hi @manuelgitgomes ,

actually, the function with the same name is in atom_core/transformations.

But there is another called compareAtomTransforms in atom_core/utilities which I also think is the same.

Fixing this wrong could have a very large impact. Might as well create a PR?

miguelriemoliveira commented 1 month ago

Hi again @manuelgitgomes ,

actually, there are differences

compareAtomTransform from utilities

https://github.com/lardemua/atom/blob/be1aeb038fc946252ab81e9e8f68623c44aeca42/atom_core/src/atom_core/utilities.py#L38

compareTransforms from transformations

https://github.com/lardemua/atom/blob/be1aeb038fc946252ab81e9e8f68623c44aeca42/atom_core/src/atom_core/transformations.py#L33

In #928 me and @brunofavs were fixing and issue with compareTransforms. Of course the fix was only pushed to one of these functions (that's the problem with having multiple functions for the same thing).

@manuelgitgomes , better that I fix it since I was working with @brunofavs .

miguelriemoliveira commented 1 month ago

Hi @manuelgitgomes ,

I just changed the computation of the translation error to use the average and not the norm, as discussed in #928. This makes the computation of error more uniform across atom functions.

I changed the compareTransforms in atom_core/transformations.py

but you have a replica (not any more) of this function in automatic_data_collector.

Please fix it. We should avoid replicating functions. When we need the function to behave differently, we create a function with a different name.

Thanks.

manuelgitgomes commented 1 month ago

Hello @miguelriemoliveira

My function was changed to use yours now:

https://github.com/lardemua/atom/blob/144c6f0fba5ddaec81668ed79bb3e2a157a84e73/atom_calibration/scripts/automatic_data_collector#L28-L50

This uses the compareTransforms and uses a threshold to maintain the same functionality as my old functions.

Thank you for bringing this to my attention

miguelriemoliveira commented 1 month ago

Great. Thanks.