-
Notifications
You must be signed in to change notification settings - Fork 3
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
Implement energy landscapes and pathways from simulation #178
Conversation
Nice work! Let me know when this is finished, and I'll happily review this code. Couple of things so far:
|
The branch has been updated and it is ready for review. In particular, the specific comments by @stefsmeets have been addressed in the following way:
Switched to networkx
I am not an expert with testing, so I would like to discuss next time about the 'best practices' in this direction
It is used to show that it does not work, as mentioned by @tfamprikis in #144. I have now moved |
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.
Looks good to me if you add a test for the perculating pathway
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.
Hi @SCiarella , Nice work! Just made a pass through the code, some small comments from my side for now. I will have a look at the notebook tomorrow and the PR as a whole! 😃
""" | ||
prob = self.data / self.data.sum() | ||
free_energy = -kBT * np.log(prob) | ||
return np.nan_to_num(free_energy) |
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.
Maybe not for this PR, but for the long term we could consider that all methods like this return an instance of Volume
with some flag that tells that it is a probability or free energy, and have some .units
attribute to keep track of the units.
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.
I just checked the notebook, and I'm wondering if you can think a bit of how you want people to use this code. I find some parts not very obvious and I think it can be streamlined quite a bit.
Feel free to tackle this in a follow-up PR if that makes more sense.
Let me know what you think!
Free energy
This is the current api in this PR:
from gemdat.transitions import optimal_path, free_energy_graph
diff_volume = trajectory_to_volume(diff_trajectory, resolution=0.3)
F = diff_volume.get_free_energy(kBT=trajectory.metadata['temperature'])
F_graph = free_energy_graph(F, max_energy_threshold=1e7, diagonal=True)
start_point = (49, 26, 10)
end_point = (10, 0, 8)
path, path_energy = optimal_path(
F_graph,
start_point,
end_point,
)
fig1 = plots.path_on_landscape(diff_volume, path, structure)
fig1.show()
fig2 = plots.energy_along_path(energy_path=path_energy)
fig2.show()
Playing around a little bit, this is what I would suggest we aim for:
- we can grab the temperature from the trajectory metadata
F
is a an instance ofVolume
or a subclass ofVolume
with energy related methods- no need for any extra gemdat imports
- does not hide the networkx graph, so users can do whatever they want with it
import networkx as nx
diff_volume = trajectory.to_volume(resolution=0.3)
F = diff_volume.to_free_energy()
G = F.to_graph(max_energy_threshold=1e7, diagonal=True)
source = (49, 26, 10)
target = (10, 0, 8)
optimal_path = nx.shortest_path(G,
source=source,
target=target,
weight='weight')
fig1 = plots.path_on_landscape(diff_volume, optimal_path, structure)
fig1.show()
energy = [G.nodes[node]['energy'] for node in optimal_path]
fig2 = plots.energy_along_path(energy_path=energy)
fig2.show()
Percolation
Same goes for the percolation, my suggestion would be for an api that looks something like this:
- no need to import anything = more discoverable
- returning a dataclass of some sort makes it easier to extend the code
- hides the wrapping in a method
peaks = F.find_peaks()
best_path = F.find_best_perc_path(peaks, perc_xyz=(True, False, False))
print(f"Total Energy required: {best_path.total_energy_cost}")
print(f"Starting Point: {best_path.starting_point}")
print(f"Best Path: {best_path.best_path}")
print(f"Best Path Energy: {best_path.best_path_energy}")
fig1 = plots.path_on_landscape(diff_volume, structure, path=best_path)
fig1.show()
fig2 = plots.energy_along_path(path=best_path)
fig2.show()
Fix plots location Update percolation function Fix missing type hints Created Pathway class
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.
Looks good to me, I suggested a few small changes, but otherwise I think its good to go, any remaining comments can be converted into issues 🚀
src/gemdat/path.py
Outdated
wrapped_coord = coord % size | ||
return wrapped_coord |
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.
I think that a function for this single statement is not necessary, and the function can be ommited
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.
Ok, I have removed the function
I have updated the branch as suggested. The remaining comments about API and usage can be discussed in another issue |
Perfect, after you create the issue, let's merge this 🚀 |
fix #144
This branch implements the analysis of optimal pathways between sites.
Only site-to-site pathways are implemented for now, but later it will be extended to identify percolating pathways and reproduce literature results.
The notebook 'paths.ipynb' shows the addition of this branch.