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

Improve API for pathways #186

Closed
SCiarella opened this issue Nov 17, 2023 · 1 comment · Fixed by #213
Closed

Improve API for pathways #186

SCiarella opened this issue Nov 17, 2023 · 1 comment · Fixed by #213

Comments

@SCiarella
Copy link
Contributor

SCiarella commented Nov 17, 2023

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 of Volume or a subclass of Volume 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()

Originally posted by @stefsmeets in #178 (review)

@SCiarella SCiarella assigned SCiarella and unassigned SCiarella Nov 17, 2023
@SCiarella SCiarella linked a pull request Feb 5, 2024 that will close this issue
@SCiarella
Copy link
Contributor Author

The API improvements that we suggest here, are being implemented in pull request #213

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 a pull request may close this issue.

1 participant