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

Rotations #220

Merged
merged 40 commits into from
Mar 25, 2024
Merged

Rotations #220

merged 40 commits into from
Mar 25, 2024

Conversation

tfamprikis
Copy link
Contributor

adding functions to track orientations issue #167

adding functions to track orientations issue #167
@stefsmeets
Copy link
Contributor

Hi @tfamprikis thanks for this! This looks like it will be quite a useful starting point. Do you plan on adding something more? Otherwise we will start integrating this into gemdat.

@stefsmeets stefsmeets marked this pull request as draft December 12, 2023 09:18
@tfamprikis
Copy link
Contributor Author

tfamprikis commented Dec 12, 2023

hey, this was just a test to see how this works.

i still have a couple of issues that havent been able to fix. mainly

  • I cant seem to be able to symmetrize my directions (created by gr.bond_direct() ). this is what I'm trying to do in lines 28-59 of test4.py. It works in matlab but not in python...
  • I've trouble with normalizing the probability plot (in polar coordinates) by the corresponding areas of the sphere, it becomes problematic near the poles, creating artifacts.

otherwise some other functions from test4.py can also be moved to gemdat_rotation (autocorr(), autocorr_par(), a function to plot distribution of bond lenghts)

I can add the data (.xml) i've been using to test to test_data and some figures of what the symmetrization looks like when it works.

all that said, if you think you can still work with what I have please go ahead...

@stefsmeets
Copy link
Contributor

Okay, no worries! The test at leas worked 😅

There is a bunch of othes stuff I still want to work on as well. I'll let you know when we get back to this.

@SCiarella SCiarella linked an issue Jan 10, 2024 that may be closed by this pull request
@SCiarella SCiarella changed the title test pull request Rotations Feb 6, 2024
@SCiarella
Copy link
Contributor

@tfamprikis Can you share the data that you used to run test4.py?
Also, have you made any further progress about the rotations that is not already here?

@tfamprikis
Copy link
Contributor Author

@tfamprikis Can you share the data that you used to run test4.py? Also, have you made any further progress about the rotations that is not already here?

Hi @SCiarella

I've uploaded the .xml + a version of the test code that actually (half)works here:
https://surfdrive.surf.nl/files/index.php/s/vVIhHHMsOU9wFT5
(let me know if you have trouble accessing)

I have not managed to make meaningful progress against the issues i had described yet...

@SCiarella SCiarella self-assigned this Feb 12, 2024
@SCiarella
Copy link
Contributor

SCiarella commented Feb 22, 2024

This pull request should fulfil the following objectives:

  • compute the orientation vectors and their trajectory given the input expected structure
  • apply the correct symmetry group to enhance the trajectory
  • produce the rotolinear plot
  • measure and plot the autocorrelation

The analysis of rotational transitions will be the object of the next pull request

Add bond length distribution plot

Add autocorrelation computation and plot
@SCiarella SCiarella marked this pull request as ready for review March 6, 2024 11:01
@SCiarella
Copy link
Contributor

You can look at this notebook to get an overview of this addition.

@SCiarella SCiarella requested a review from stefsmeets March 6, 2024 13:17
Copy link
Contributor

@stefsmeets stefsmeets left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @SCiarella thanks for fixing up this PR! It looks like all the bits are in place, the plots look great, but I think it could use some polish before merging. I made a first pass through the code, please see my comments.

This seems to be the basic usage:

bd = Orientations(trajectory, central_atoms='S', satellite_atoms='O', n_expected_neigh=8)

direction_cart = bd.get_conventional_form(normalize=False)
direction_spherical = bd.cartesian_to_spherical(direct_cart=direction_cart, degrees=True)

plots.rectilinear_plot(trajectory=direction_spherical, normalize=False)

sg = Oh_point_group()
direction_cart_sym =  bd.get_symmetric_traj(sg.sym_ops_Oh)
direction_spherical_sym = bd.cartesian_to_spherical(direct_cart=direction_cart_sym, degrees=True)

plots.rectilinear_plot(trajectory=direction_spherical_sym, normalize=False)
  • I'm wondering if we can clean up the api a little bit, so it's a bit more inviting to use
  • Have you looked at the pymatgen symmetry module? There seems to be a lot of code that can help with generating symmetry operations for point groups. This could replace Oh_point_group with familiar pymatgen code.
  • Can you also add some unit tests for the new code?
  • Can you refactor the Orientations class to get rid of some of the superfluous methods(see comments)?

src/gemdat/plots/__init__.py Outdated Show resolved Hide resolved
src/gemdat/plots/matplotlib/_rotations.py Outdated Show resolved Hide resolved
src/gemdat/plots/matplotlib/_rotations.py Outdated Show resolved Hide resolved
src/gemdat/plots/matplotlib/_rotations.py Outdated Show resolved Hide resolved
src/gemdat/plots/matplotlib/_rotations.py Outdated Show resolved Hide resolved
src/gemdat/rotations.py Outdated Show resolved Hide resolved
src/gemdat/rotations.py Outdated Show resolved Hide resolved
src/gemdat/rotations.py Outdated Show resolved Hide resolved
src/gemdat/rotations.py Show resolved Hide resolved
src/gemdat/rotations.py Outdated Show resolved Hide resolved
@stefsmeets
Copy link
Contributor

stefsmeets commented Mar 13, 2024

We could use:

from pymatgen.symmetry.groups import PointGroup
g = symmetry.groups.PointGroup('m-3m')  # Oh
len(g.symmetry_ops)
# 48
g.symmetry_ops[0]
# SymmOp(affine_matrix=array([[ 0.,  0., -1.,  0.],
#       [ 0., -1.,  0.,  0.],
#       [-1.,  0.,  0.,  0.],
#       [ 0.,  0.,  0.,  1.]]))

SCiarella and others added 2 commits March 14, 2024 08:42
Co-authored-by: Stef Smeets <stefsmeets@users.noreply.github.com>
Co-authored-by: Stef Smeets <stefsmeets@users.noreply.github.com>
@SCiarella
Copy link
Contributor

I have cleaned the api, so now the basic usage looks like:

bd = Orientations(trajectory, central_atoms='S', satellite_atoms='O', n_expected_neigh=8, normalize_trajectory=False)

plots.rectilinear_plot(data=bd, symmetrize=False)

bd.set_symmetry_operations(sym_group='m-3m')

plots.rectilinear_plot(data=bd, symmetrize=True)

which is now using pymatgen.PointGroup to get the symmetry, so I have removed Oh_point_group.
I have also refactored and removed some superfluous methods as suggested.

@SCiarella SCiarella marked this pull request as ready for review March 15, 2024 10:26
@SCiarella SCiarella requested a review from stefsmeets March 15, 2024 11:00
Copy link
Contributor

@stefsmeets stefsmeets left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, using the symmetries from pymatgen makes this a lot cleaner. A few small final comments from my side.

src/gemdat/plots/matplotlib/_rotations.py Outdated Show resolved Hide resolved
src/gemdat/plots/matplotlib/_rotations.py Outdated Show resolved Hide resolved
src/gemdat/rotations.py Outdated Show resolved Hide resolved
src/gemdat/plots/matplotlib/_rotations.py Outdated Show resolved Hide resolved
src/gemdat/plots/matplotlib/_rotations.py Outdated Show resolved Hide resolved
src/gemdat/plots/matplotlib/_rotations.py Outdated Show resolved Hide resolved
src/gemdat/rotations.py Outdated Show resolved Hide resolved
src/gemdat/rotations.py Outdated Show resolved Hide resolved
src/gemdat/rotations.py Outdated Show resolved Hide resolved
src/gemdat/rotations.py Outdated Show resolved Hide resolved
SCiarella and others added 2 commits March 18, 2024 11:28
Co-authored-by: Stef Smeets <stefsmeets@users.noreply.github.com>
Co-authored-by: Stef Smeets <stefsmeets@users.noreply.github.com>
@SCiarella SCiarella marked this pull request as draft March 18, 2024 10:29
SCiarella and others added 10 commits March 18, 2024 11:29
Co-authored-by: Stef Smeets <stefsmeets@users.noreply.github.com>
Co-authored-by: Stef Smeets <stefsmeets@users.noreply.github.com>
Co-authored-by: Stef Smeets <stefsmeets@users.noreply.github.com>
Co-authored-by: Stef Smeets <stefsmeets@users.noreply.github.com>
Co-authored-by: Stef Smeets <stefsmeets@users.noreply.github.com>
Co-authored-by: Stef Smeets <stefsmeets@users.noreply.github.com>
Rename data -> orientations
@SCiarella SCiarella marked this pull request as ready for review March 18, 2024 12:04
@SCiarella SCiarella requested a review from stefsmeets March 18, 2024 12:07
@tfamprikis
Copy link
Contributor Author

Hi guys, I've been playing with this a while. some implementations don't really make sense to me rn. should I comment here or wait for this to be merged and make issues?

@SCiarella
Copy link
Contributor

@tfamprikis I think this pull request will be closed relatively soon (since it is also quite long), so it would be nice if you could open new issues to address all the problems that you see.

Copy link
Contributor

@stefsmeets stefsmeets left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

@tfamprikis I agree with @SCiarella that it's good to get this merged now and fix remaining issues later.

@stefsmeets stefsmeets merged commit 0e7f646 into main Mar 25, 2024
5 checks passed
@stefsmeets stefsmeets deleted the tfamprikis-test-dontlook branch March 25, 2024 09:46
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.

Basic functions of rotation implementation
3 participants