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

Bug..fix? #87

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

lakshmipathyarjun6
Copy link

It seems like the log map computation did not actually take the orientation of a tangent vector into account. Tried this out with the vector heat demo and this appears to adjust with the correct tangent vector propagation...but not entirely sure why the sign inversion was necessary / why the sweep started from the other side if the real axis (presumably) is oriented in the tv direction.

Adding defaulting to not make other stuff blow up :)

Feel free to reject if unwarranted!

@nmwsharp
Copy link
Owner

nmwsharp commented May 7, 2021

Thanks for submitting! This seems like a reasonable extension to me.

I think the sign inversion should not actually be needed. You might be getting tricked by the Polyscope visualization in the vector heat demo. There, the "branch cut" discontinuity in the angular color ends up at the -x axis, since it's computed via atan.

@lakshmipathyarjun6
Copy link
Author

eh...why it not like 1i? and ah makes sense - will remove the inversion and resubmit

@nmwsharp
Copy link
Owner

nmwsharp commented May 7, 2021

I think i is not defined as a constant on all platforms... std::complex<double>{real_part, imag_part} always works

@lakshmipathyarjun6
Copy link
Author

ah. got it

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 this pull request may close these issues.

2 participants