-
Notifications
You must be signed in to change notification settings - Fork 188
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
Cylinder transformation and observables #4114
Conversation
src/python/espressomd/observables.py
Outdated
kwargs["orientation"] = vec / np.linalg.norm(vec) | ||
orthogonal_component = vec - np.dot(vec, axis) * axis | ||
norm = np.linalg.norm(orthogonal_component) | ||
if norm > 0.01: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is this magic 0.01 here? do want to check for sys.float_info.epsilon
, maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we replicate the logic from the core here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1.) if the orthogonal component is 2 epsilon long and we scale it to 1, it will probably not be very orthogonal due to precision error. I chose a very generous value of 0.1 because we always have the second vector to try which will be more orthogonal. I guess using 1/sqrt(2) as a threshold will choose the vector that is the most orthogonal already and remove some arbitraryness
2.) The core logic for default orientation is for hollowconicalfrustum, the python one for observables
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- okay
- i think the logic is in the wrong place then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where would you suggest is the right place to unite it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
algorithmically I think we could do something like: https://math.stackexchange.com/a/1681815
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but that is what I am doing here :)
The Gram-Schmidt-type orthogonalisation works in whatever dimension
You just have to start with a trial vector that is not parallel
I know I still have to adapt the tutorial and do the documentation, I will do that as soon as I get green light on the stuff that is here |
could you please fix the sanitizer warnings |
@christophlohrmann could you please take the final steps here? We can then merge this soon I think... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a few formatting issues
Co-authored-by: Jean-Noël Grad <[email protected]>
24930cd
to
0e67661
Compare
Fixes #4023, fixes #4067
Description of changes:
Adds automatic
orientation
to core HollowConicalFrustumReworks the cylindrical observables tests
Fixes the LB fluxdensity observable by implementing a LB density interpolation
introduces a new object
CylTrafoParams
to make sureaxis
andorientation
are always orthogonalall cylindrical observables use
CylTrafoParams