-
-
Notifications
You must be signed in to change notification settings - Fork 38
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
[REVIEW]: MD-SAPT: Python Based Toolkit for Running Symmetry Adapted Perturbation Theory Calculations on Molecular Dynamics Trajectories #5931
Comments
Hello humans, I'm @editorialbot, a robot that can help you with some common editorial tasks. For a list of things I can do to help you, just type:
For example, to regenerate the paper pdf after making changes in the paper's md or bib files, type:
|
|
|
Wordcount for |
@wiederm, @CarvFS – This is the review thread for the paper. All of our communications will happen here from now on. Please read the "Reviewer instructions & questions" in the first comment above. Please create your checklist typing:
As you go over the submission, please check any items that you feel have been satisfied. There are also links to the JOSS reviewer guidelines. The JOSS review is different from most other journals. Our goal is to work with the authors to help them meet our criteria instead of merely passing judgment on the submission. As such, the reviewers are encouraged to submit issues and pull requests on the software repository. When doing so, please mention We aim for the review process to be completed within about 4-6 weeks but please make a start well ahead of this as JOSS reviews are by their nature iterative and any early feedback you may be able to provide to the author will be very helpful in meeting this schedule. |
Review checklist for @CarvFSConflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
|
Review: observations on the paper and functionalityFirst of all: thank you very much for developing this toolkit! It is important to make the non-convalent bond analysis easier for everyonw working with molecular dynamics. Here I will post some comments on the paper and toolkit. Any new observations I will post as a new comment. About installation:I think the authors should update the documentation to avoid users running into issues while trying to install their toolkit. A few things I have observed:
Maybe using some specific version of pydantic would avoid my last error. However, it should be listed in the documentation which versions the user must have. About important links
About the paper and toolkit
General comments
|
👋 @wiederm – just checking in here, when do you think you might be able to complete your review? |
Hi @arfon , sorry for the delay! I will add my review on Monday! |
Review checklist for @wiedermConflict of interest
Code of Conduct
General checks
Functionality
Documentation
Software paper
|
ReviewThank you for providing this software. Software
Manuscriptl40: comment needs to be removed
|
Additional review:Generating input file:
and this cause an error. For example, by running:
and then,
I have got the error:
By filling The way it is described in the paper, the reader will be lead to think that one just need to run
Running SAPT calculations
To make it run I have changed to
This should be fixed in the paper example. |
@armcdona – looks like there's a fair amount of feedback to start working on from the reviewers here. Could you get started and let us know here how you're getting on? |
@armcdona 👋 we hope you are doing well. Could you please provide an update to the editor here? Thanks! |
I'm currently working on packaging it with conda-forge (conda-forge/staged-recipes#24782). We were previously packaged with psi4's channel but since they're distributing with conda-forge now, we're moving our package there too. I've been very busy with work recently, and had to deal with a cold and strep, but I will get back to packaging this soon. |
👋 @ifd3f – just checking in again here to see how you're getting along? |
I got builds to work, now I need to make it pass the conda-forge CI. Hopefully I should have it published on conda-forge by end of week. Sorry about the wait. |
@ifd3f thanks for that update. Were you able to get that done? Let us know if we can proceed. It would be good to proceed, to avoid loosing track of the reviewers. |
It was finally merged into conda-forge after a lengthy process. I'll update the instructions in the paper once I get a chance. |
Hi @arfon @Kevin-Mattheus-Moerman, sorry for the late update, but we are ready to proceed again. |
@ifd3f - are you suggesting you're ready for the reviewers to pick up their reviews again? If so, could you please give a brief summary of the updates you've made in response to their feedback thus far? |
I wanted to apologize for the delays, I've been dealing with some mental and physical health issues, but am committed to seeing this process through. I started my response to the reviews today, and will be putting out updates to the software and paper throughout the week. So It's easier to read, and I don't blow up everyone's notifications, I'll give the responses as a few larger comments on the thread. |
Response to First @CarvFS ReviewThis is a summary of my response to the feedback in the first review in updates both to the paper and MD-SAPT its self. Addressing Instillation issues@ifd3f has worked on resolving the installation issues, and getting an official Conda-Forge release.
We dropped 3.7 support, but I think we must have forgotten to remove that from out list of supported versions.
The Conda-Forge release should have resolved, you can install with MDSAPT using Addressing Important Links
I fixed the links in the paper.
I updated the demo's repo to use conda release of MD-SAPT, but for some reason it installs a borken version of psi4. Addressing Paper and Software Concerns
I updated the information about the steps out workflow includes to be more clear. It now reads:
Do you think that it describes the process more clearly?
I think that paragraph was unclear, we were indeding to discuss existing tools for working with MD data, and how MD-SAPT fits in with those.
The next paragraph elaborates on how MD-SAPT is an MDAKit, which is a part of the MDAnalysis ecosystem.
Yes, it is by modifying the
That is a good idea for making it more clear how to use MD-SAPT so I added some information to the input files generated using This is the new format of the trajectory analysis file: psi4:
# The sapt theory level eg: SAPT0, SAPT2, SAPT2+, SAPT2+3
# See https://psicode.org/psi4manual/master/sapt.html#sapt-level for more information
method: 'sapt0'
# Basis set for Psi4 SAPT calculation
# See https://psicode.org/psi4manual/master/basissets_byelement.html#apdx-basiselement for complete list
basis: 'jun-cc-pvdz'
# set to true to save Psi4 output files
save_output: true
# Additional Psi4 settings see https://psicode.org for more psi4 specific documentation
settings:
# The reference wave function type eg: RHF, ROHF, UHF, CUHF, RKS, UKS
# See https://psicode.org/psi4manual/master/autodir_options_c/scf__reference.html for more information
reference: 'rhf'
simulation:
ph: 7.0
# pH of simulated system, it informs how protons are replaced when balancing spin state before Psi4 calculations
charge_guesser: 'standard'
# charge_guesser: 'rdkit' # to use rdkit. Make sure it is installed first.
system_limits:
# Integer number of cpu cores to use in Psi4 calculation
ncpus:
# Given memory form `"{number_of_gigabytes}GB"`
memory:
analysis:
# This section is for running TrajectorySAPT. To run other types of analyses, see below.
type: 'trajectory'
topology: 'your/file/path.pdb'
# If you want to override the charges of specific, you may specify it this way.
# topology:
# path: 'your/file/path.pdb'
# charge_overrides:
# 132: -2
trajectories:
- 'your/trajectory/path.dcd'
pairs:
# List the pairs of residues to calculate interaction energy between based on residue ids in topology file
- [132, 152]
- [34, 152]
frames:
# Starting frame
start: 10
# Stop frame
stop: 30
# frames per calculation step
step: 1
# Name of outputed csv file
output: 'output.csv'
This is what the default docking input file gives you: psi4:
# The sapt theory level eg: SAPT0, SAPT2, SAPT2+, SAPT2+3
# See https://psicode.org/psi4manual/master/sapt.html#sapt-level for more information
method: 'sapt0'
# Basis set for Psi4 SAPT calculation
# See https://psicode.org/psi4manual/master/basissets_byelement.html#apdx-basiselement for complete list
basis: 'jun-cc-pvdz'
# set to true to save Psi4 output files
save_output: true
# Additional Psi4 settings see https://psicode.org for more psi4 specific documentation
settings:
# The reference wave function type eg: RHF, ROHF, UHF, CUHF, RKS, UKS
# See https://psicode.org/psi4manual/master/autodir_options_c/scf__reference.html for more information
reference: 'rhf'
simulation:
ph: 7.0
# pH of simulated system, it informs how protons are replaced when balancing spin state before Psi4 calculations
charge_guesser: 'standard'
# charge_guesser: 'rdkit' # to use rdkit. Make sure it is installed first.
system_limits:
# Integer number of cpu cores to use in Psi4 calculation
ncpus:
# Given memory form `"{number_of_gigabytes}GB"`
memory:
analysis:
type: 'docking'
# MD-SAPT analysis type (in this case docking)
topology_directory: 'mek/docking'
# The set of topology files for docking analysis
pairs:
# Place pair names defined above in list in a list
- [15, 132]
system_limits:
# Integer number of cpu cores to use in Psi4 calculation
ncpus:
# Given memory form `"{number_of_gigabytes}GB"`
memory:
I think this is a question that is could be better anwsered by @armcdona, as she is a quantum chemist and far more knowledgeable on psi4 than I am.
I'll run some tests but looking at the Psi4 documentation, I see no reason why providing
Thank you for the papers you attached with this feedback, they were very helpful for demonstrating the utility of MD-SAPT's methodology. Here is what I wrote verbatim: "The molecular mechanics methods used in MD simulations do not characterize noncovalent interactions at the same level of detail as quantum mechanical methods, but allow simulations of large biomolecular systems [@lipkowitz]. I hope that this better describes why MD-SAPT may be useful to an audience outside of quantum chemistry.
I argee that was vague wording, and I made it clear that the data in that figure is what MD-SAPT generates. This is the replacement sentence:
I have not specifically tested it but we can use any format that MDAnalysis supports, so amber's .prmtop should work with current versions of MDAnalysis. Other Questions
We have since then made some changes to the repository based on reviews and fixing releases. |
Thanks for the update and long response @ALescoulie. @wiederm, @CarvFS – when you have a moment, would you both be able to update your reviews based on this feedback? |
Hi @arfon and @ALescoulie! I hope you are better now, @ALescoulie :) and thank you very much for your reply! I will try the install process later to see if I get any issues, but everything else covered in the reply sounds excellent to me. After getting the feedback on my second review I will update the review checklist. |
Submitting author: @armcdona (Ashley Ringer McDonald)
Repository: https://github.com/calpolyccg/MDSAPT
Branch with paper.md (empty if default branch): JOSS_Pub
Version: v2.0
Editor: @arfon
Reviewers: @wiederm, @CarvFS
Archive: Pending
Status
Status badge code:
Reviewers and authors:
Please avoid lengthy details of difficulties in the review thread. Instead, please create a new issue in the target repository and link to those issues (especially acceptance-blockers) by leaving comments in the review thread below. (For completists: if the target issue tracker is also on GitHub, linking the review thread in the issue or vice versa will create corresponding breadcrumb trails in the link target.)
Reviewer instructions & questions
@wiederm & @CarvFS, your review will be checklist based. Each of you will have a separate checklist that you should update when carrying out your review.
First of all you need to run this command in a separate comment to create the checklist:
The reviewer guidelines are available here: https://joss.readthedocs.io/en/latest/reviewer_guidelines.html. Any questions/concerns please let @arfon know.
✨ Please start on your review when you are able, and be sure to complete your review in the next six weeks, at the very latest ✨
Checklists
📝 Checklist for @CarvFS
📝 Checklist for @wiederm
The text was updated successfully, but these errors were encountered: