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

Add peak labels to free energy plots #213

Merged
merged 16 commits into from
Feb 6, 2024
Merged

Conversation

SCiarella
Copy link
Contributor

This branch covers #203.
I have used a KD-tree to precompute a dictionary that maps each site to the closest peak.
Then after finding the path, I implemented a method to use the dictionary to identify which peaks get explored.
This information gets automatically plotted on the x-axis of the energy landscape:
fig1
fig2

@SCiarella SCiarella changed the title Add peak lable to free energy plots Add peak labels to free energy plots Nov 24, 2023
@stefsmeets
Copy link
Contributor

To my understanding the goal for the related issue is to link the coordinates back to the site labels from the cif file.

i.e. 48h instead of (28, 15, 10)

@stefsmeets stefsmeets self-requested a review November 29, 2023 08:14
@stefsmeets stefsmeets marked this pull request as draft November 29, 2023 08:14
@SCiarella
Copy link
Contributor Author

Now the path is compared to the real structure obtained from load_known_material(...) instead of the peaks of the density.
The figure have been updated in order to plot the site label on the top, and its Cartesian coordinates on the bottom:

1
2

@SCiarella SCiarella marked this pull request as ready for review December 5, 2023 13:50
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 this PR. I just had a look and left some feedback. The plots look nice, though I'm wondering if it makes sense that the diffusing atom passes through so many cages so quickly.

My overall feeling is that this PR is not complete yet, and I'm not quite sure how this is supposed to be used.

  • Can you add some tests for the plot (see tests/integration/plot_test.py)?
  • Can you add some instructions for how this code is supposed to be used?
  • The kdtree looks for the nearest atom for every voxel, shouldn't there be based some sort of distance threshold?

src/gemdat/path.py Outdated Show resolved Hide resolved
src/gemdat/path.py Outdated Show resolved Hide resolved
src/gemdat/path.py Outdated Show resolved Hide resolved
src/gemdat/path.py Outdated Show resolved Hide resolved
src/gemdat/plots/matplotlib/_paths.py Outdated Show resolved Hide resolved
src/gemdat/volume.py Outdated Show resolved Hide resolved
src/gemdat/volume.py Outdated Show resolved Hide resolved
src/gemdat/volume.py Outdated Show resolved Hide resolved
src/gemdat/volume.py Outdated Show resolved Hide resolved
src/gemdat/volume.py Outdated Show resolved Hide resolved
Add optional args to Pathways

Move structure map into volume object

Implement fractional coords for Pathways

Correct for orthorombic lattices using pymatgen core

Update test for new rounding errors
Add pytest fixtures

Add path plots tests
@SCiarella
Copy link
Contributor Author

As discussed during the meeting on 09/01/24, I have implemented a color-coding to easily distinguish the closest site to the path:
a

In this last series of commits, I have also added a test for this plotting function.

@SCiarella SCiarella linked an issue Jan 10, 2024 that may be closed by this pull request
SCiarella and others added 2 commits January 18, 2024 17:00
* Add bellman-ford pathfinding algo

Add dijkstra-exp pathfinding algo

Add minmax-energy pathfinding algo

Add testing for extra pathfinding

* Update src/gemdat/path.py

Rearrange weight_exp

Co-authored-by: Stef Smeets <[email protected]>

* Update src/gemdat/path.py

replace stacked 'or' operators

Co-authored-by: Stef Smeets <[email protected]>

* Update src/gemdat/path.py

replaced stacked 'or' operators (2)

Co-authored-by: Stef Smeets <[email protected]>

* Move minmax-energy pathfinding to separate function

---------

Co-authored-by: Stef Smeets <[email protected]>
@stefsmeets
Copy link
Contributor

I'll review this next week when I'm back!

@stefsmeets stefsmeets force-pushed the label-landscape-sites branch from 7aa311f to 68606b7 Compare January 31, 2024 09:35
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.

I don't understand what is going on in this PR, it also includes the changes from #239, which makes this difficult to review. I tried to rebase to main to get rid of the already merged PR, but this did not work.

My main concern is that it is still not clear to me how this is supposed to be used. This api does not make sense to me. How would a user know they would need to run this?

structure = load_known_material('argyrodite')
vol.nearest_structure_reference(structure)
path.cartesian_path(vol)
path.fractional_path(vol)
path.path_over_structure(structure, vol)
plots.energy_along_path(path=path)

vs.

plots.energy_along_path(path=path)

What is the difference?

This is of course also a bit tied to #186. I'm also happy to have a go at updating the code. Otherwise let me know if you want to have a short chat about it!

src/gemdat/volume.py Outdated Show resolved Hide resolved
src/gemdat/path.py Outdated Show resolved Hide resolved
src/gemdat/path.py Outdated Show resolved Hide resolved
src/gemdat/volume.py Outdated Show resolved Hide resolved
src/gemdat/path.py Outdated Show resolved Hide resolved
@SCiarella SCiarella force-pushed the label-landscape-sites branch from 68606b7 to fdb06a7 Compare February 5, 2024 09:56
@SCiarella SCiarella marked this pull request as draft February 5, 2024 10:42
@SCiarella SCiarella linked an issue Feb 5, 2024 that may be closed by this pull request
Update tests

Fix bug in fractional coordinates labels
@SCiarella SCiarella marked this pull request as ready for review February 5, 2024 12:56
@SCiarella
Copy link
Contributor Author

Notice that I have not implemented the @cache decorator for fractional_path, cartesian_path and nearest_structure_reference.
The implementation is not trivial because our custom Volume and Pathway classes are not hashable.

@SCiarella SCiarella requested a review from stefsmeets February 5, 2024 13:04
@stefsmeets stefsmeets merged commit cd91c63 into main Feb 6, 2024
3 checks passed
@stefsmeets stefsmeets deleted the label-landscape-sites branch February 6, 2024 09:34
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.

Energy landscape: Add site labels to energy plot Improve API for pathways
2 participants