ami-iit / adam

adam implements a collection of algorithms for calculating rigid-body dynamics in Jax, CasADi, PyTorch, and Numpy.
https://adam-docs.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
123 stars 19 forks source link

Wrong return type hint in parametric get_total_mass #69

Closed S-Dafarra closed 5 months ago

S-Dafarra commented 6 months ago

I noticed that the parametric version of get_total_mass has float as return type hint, but it returns a Casadi function instead.

Moreover, there are two consecutive return. See https://github.com/ami-iit/adam/blob/2f18e9e737beeb00a4687a37d10a4822d0e5959b/src/adam/parametric/casadi/computations_parametric.py#L272-L275

cc @CarlottaSartore

S-Dafarra commented 6 months ago

In addition, the input order is different. For all the other functions, the length multiplier input comes before the densities. For this function only, it is inverted.

At this point, I think it is safer to rename that method to get_total_mass_fun so that if some other code was using it, it is clearer that it has changed. In fact, both the two inputs are vectors, and it might be possible to detect easily when they are swapped.

S-Dafarra commented 6 months ago

In addition, the input order is different. For all the other functions, the length multiplier input comes before the densities. For this function only, it is inverted.

At this point, I think it is safer to rename that method to get_total_mass_fun so that if some other code was using it, it is clearer that it has changed. In fact, both the two inputs are vectors, and it might be possible to detect easily when they are swapped.

Interestingly, the test in https://github.com/ami-iit/adam/blob/2f18e9e737beeb00a4687a37d10a4822d0e5959b/tests/parametric/test_CasADi_computations_parametric.py#L127C62-L127C78 does not fail. I guess this is because the total mass is bilinear on the density and length, so maybe inverting their order does not change 🤔

S-Dafarra commented 6 months ago

I fixed it in https://github.com/ami-iit/adam/pull/72