-
Notifications
You must be signed in to change notification settings - Fork 6
Python script for analyzing TSH trajectories from ABIN. #206
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
Conversation
The script plots PES along the trajectory, electronic populations, total and kinetic energy from either LZSH or FSSH. All is in one plot and allows analysis of a single trajectory run.
Hi @danielhollas, I've created a Python script which I use to analyze TSH calculations in ABIN. I thought it might be handy to attach it to ABIN as people could use it for analysis. Please, have a look and let me know if you fancy having such a script with ABIN. |
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.
Pull Request Overview
This PR introduces a Python script for analyzing TSH trajectories from ABIN simulations by plotting potential energy surfaces, electronic populations, and energy data from either LZSH or FSSH runs.
- Adds a command-line interface for specifying input files and options for energy unit conversion and curve shifting.
- Implements file existence checks and plotting of trajectories and energy profiles based on generated data.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #206 +/- ##
=======================================
Coverage 84.05% 84.05%
=======================================
Files 47 47
Lines 6599 6599
Branches 764 764
=======================================
Hits 5547 5547
Misses 853 853
Partials 199 199
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Yes please! I'll have a closer look later but this looks very handy of course! 👏 😍 (I might do a quick pass over the script if you don't mind. But let me know if you prefer to iterate over review) |
Go for it, no need to iterate over review. |
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.
This is great, thanks! I made some insignificant changes and added inline metadata so that the script can be run via uv run
. I have some questions / suggestions about the command line arguments.
analysis/tsh_traj_anal.py
Outdated
#!/bin/env python3 | ||
# /// script | ||
# requires-python = ">=3.9" | ||
# dependencies = [ |
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.
Added via
uv add --script tsh_traj_anal.py numpy matplotlib PyQT6
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.
Just a question, why is PyQT6 needed 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.
Good question. When using python installations from uv, the plt.show
command did not work for me (no graph was interactively displayed). This is a known issue: astral-sh/uv#6893
So this is a workaround for that. It's not ideal, but I think better then potentially failing script. I'll add a comment about it.
I modified the script according to your comments plus I added some checks for the nstates variable. Please, have a look. |
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.
Thank you for the changes! I have two questions about the energy shifting, but otherwise I am happy, this will be very useful!
I don't think we need a new top-level analysis
directory at this point, so I moved it to utils/
. At some point we might want to organize the utils/
directory a bit better, but not here.
utils/analysis/tsh_traj_anal.py
Outdated
parser.add_argument( | ||
"-es", | ||
"--energy-shift", | ||
action="store_false", | ||
help="Shift curves so that the lowest state minimum has 0 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.
This option is currently confusing to me. If I understand correctly, specifying this option actually turns off the energy shift?
What if we instead have an option that specifies the zero energy (probably in atomic units)? For example
parser.add_argument( | |
"-es", | |
"--energy-shift", | |
action="store_false", | |
help="Shift curves so that the lowest state minimum has 0 energy", | |
) | |
parser.add_argument( | |
"-ze", | |
"--zero-energy", | |
type=float | |
default=None, | |
help="Specify zero energy (in atomic units). All energy curves will be shifted with respect to this energy.", | |
) |
To turn off energy shifting, the user then could pass (--energy-shift 0
).
And this would also allow to pass any value, which would be handy if you were analyzing multiple trajectories and wanted to have consistent E0)
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.
That's a good point. I added it ages ago and probably used it once in my life. Your suggestion makes complete sense.
utils/analysis/tsh_traj_anal.py
Outdated
|
||
# Shift energies | ||
if config.energy_shift: | ||
minE = np.min(data.T[1:, :]) |
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 did not notice this before, but this looks like the E0 is taken as the minimum S0 energy across the whole trajectory? To me this is a non-obvious choice. I would expect to simply shift the energy based on S0 at t0. What do you 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.
Yep, that's also a good point. I would modify it to the minimum of PES at t0. If we have triplets in LZ, the lowest triplet could be below S0, e.g., for oxygen. But otherwise it should be the same as taking S0 energy at t0.
I worked on the suggestions regarding the |
Perfect! That's actually what I had in mind, but didn't write out clearly apparently. Thanks for bearing with me. :-) |
The script plots PES along the trajectory, electronic populations, total and kinetic energy from either LZSH or FSSH. All is in one plot and allows analysis of a single trajectory run.