Skip to content
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

Wrong return type hint in parametric get_total_mass #69

Closed
S-Dafarra opened this issue Feb 26, 2024 · 3 comments · Fixed by #72
Closed

Wrong return type hint in parametric get_total_mass #69

S-Dafarra opened this issue Feb 26, 2024 · 3 comments · Fixed by #72

Comments

@S-Dafarra
Copy link
Member

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

return cs.Function(
"m", [self.densities, self.length_multiplier], [m], self.f_opts
)
return self.rbdalgos.get_total_mass()

cc @CarlottaSartore

@S-Dafarra
Copy link
Member Author

S-Dafarra commented Feb 27, 2024

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
Copy link
Member Author

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
Copy link
Member Author

I fixed it in #72

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant