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

Do not assume that there is no memory aliasing between output of mutating functions and their input arguments. #393

Closed
mateuszbaran opened this issue Jun 25, 2021 · 5 comments · Fixed by #394

Comments

@mateuszbaran
Copy link
Member

It can lead to hard-to-find bugs, see #392 .

@kellertuer
Copy link
Member

I agree, the should not assume that.
But this is phrased quite vague – should we add the test I did generally to test_manifold to have this in mind?

@mateuszbaran
Copy link
Member Author

It's a bit vague because I'm not sure what exactly should be done. Checking that all functions on all manifolds correctly handle aliasing solves this issue but it would add a relatively long time to each CI run.

@kellertuer
Copy link
Member

It would not be all functions, but sure, epx/log/retract/inverse_retracte/vector_transport_* at least.

@kellertuer
Copy link
Member

So lets make the list precise

  • embed!
  • exp!
  • inverse_retract
  • log!
  • mid_pont!
  • project!
  • retract!
  • vector_transport_to!
  • vector_transport_direction!
  • vector_transport_along! (maybe not since we do not have good generic tests until now I think)

I think we do not have to check zero_vector!. I am not sure about pole_ladder! and shields_ladder! since they are generic in ManifoldsBase. We could introduce a parameter in test_manifold to activate them?

@mateuszbaran
Copy link
Member Author

Thanks for the list, checking them makes sense. A parameter in test_manifold that activates them makes sense.

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.

2 participants