-
Notifications
You must be signed in to change notification settings - Fork 77
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Add apo ACE2 and SARS RBD:ACE2 structures #31
base: master
Are you sure you want to change the base?
Conversation
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.
Hello,
See my inline comments.
They could help you to reduce the technical debt of these script and help you to reduce the maintenance cost.
|
||
# Write initial model | ||
print('Writing initial solvated system to %s' % solvated_pdb_filename) | ||
with open(output_prefix + solvated_pdb_filename, 'w') as outfile: |
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.
You could use os.path.join
to avoid possible issue with path handling:
import os
... # code between import and the statement below
with open(os.path.join(output_prefix, solvated_pdb_filename), 'w') as outfile:
# Serialize integrator | ||
print('Serializing integrator to %s' % integrator_xml_filename) | ||
integrator = openmm.LangevinIntegrator(temperature, collision_rate, timestep) | ||
with open(output_prefix + integrator_xml_filename, 'w') as outfile: |
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.
Same thing here, prefer to use os.path.join
elapsed_time = (time.time() - initial_time) * unit.seconds | ||
simulation_time = niterations * nsteps * timestep | ||
print(' Equilibration took %.3f s for %.3f ns (%8.3f ns/day)' % (elapsed_time / unit.seconds, simulation_time / unit.nanoseconds, simulation_time / elapsed_time * unit.day / unit.nanoseconds)) | ||
with open(output_prefix + equilibrated_pdb_filename, 'w') as outfile: |
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.
Same thing here, prefer to use os.path.join
# Serialize state | ||
print('Serializing state to %s' % state_xml_filename) | ||
state = context.getState(getPositions=True, getVelocities=True, getEnergy=True, getForces=True) | ||
with open(output_prefix + state_xml_filename, 'w') as outfile: |
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.
Same thing here, prefer to use os.path.join
# Serialize system | ||
print('Serializing System to %s' % system_xml_filename) | ||
system.setDefaultPeriodicBoxVectors(*state.getPeriodicBoxVectors()) | ||
with open(output_prefix + system_xml_filename, 'w') as outfile: |
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.
Same thing here, prefer to use os.path.join
# Serialize integrator | ||
print('Serializing integrator %s' % output_prefix + integrator_xml_filename) | ||
integrator = LangevinIntegrator(temperature, collision_rate, timestep, splitting) | ||
with open(output_prefix + integrator_xml_filename, 'w') as outfile: |
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.
Same thing here, prefer to use os.path.join
outfile.write(xml) | ||
|
||
print('Loading OpenMM State...') | ||
with open(output_prefix + state_load_xml_filename, 'r') as infile: |
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.
Same thing here, prefer to use os.path.join
elapsed_time = (time.time() - initial_time) * unit.seconds | ||
simulation_time = niterations * nsteps * timestep | ||
print(' Equilibration took %.3f s for %.3f ns (%8.3f ns/day)' % (elapsed_time / unit.seconds, simulation_time / unit.nanoseconds, simulation_time / elapsed_time * unit.day / unit.nanoseconds)) | ||
with open(output_prefix + equilibrated_pdb_filename, 'w') as outfile: |
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.
Same thing here, prefer to use os.path.join
# Serialize state | ||
print('Serializing state to %s' % output_prefix + state_xml_filename) | ||
state = context.getState(getPositions=True, getVelocities=True, getEnergy=True, getForces=True) | ||
with open(output_prefix + state_xml_filename, 'w') as outfile: |
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.
Same thing here, prefer to use os.path.join
# Serialize system | ||
print('Serializing System to %s' % output_prefix + system_xml_filename) | ||
system.setDefaultPeriodicBoxVectors(*state.getPeriodicBoxVectors()) | ||
with open(output_prefix + system_xml_filename, 'w') as outfile: |
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.
Same thing here, prefer to use os.path.join
Instead of |
Why not... as long we surround path construct with something I'm happy :) |
Also, in the sake to keep a proper project history (git history), it could be worth to squash your commits related to this pull request. $ git rebase -i HEAD~4 # Will open an editor
pick 7506417 add ace2 apo and sars rbd:ace2 prepped structures and update READMEs
fixup 6d6917f fix icloud sync issues and update system_preparation README
fixup 92f3c93 add 6m17_ace2 and 2ajf_sars equilibrated structures
fixup c041f14 add 2ajf_ace2 equilibrated
$ git push -f origin add-structures Maybe the commit message could be reworded to reflect the 3 commits that added something (the fix could be dropped from the rewording). It could help us to keep/introduce a more semantical project history for users/developers. |
@4383: Thanks for the feedback! These scripts were one-off scripts written in a real hurry, and don't necessarily reflect best practices, but this feedback is super valuable as we develop an automated pipeline that implements refinements of the scheme we used to set these projects up! |
Thanks for your feedback, I suspected that these changes was made under rushing period, feel free to ignore them if you think they parasityzing your calendar or if they are not you priority for now, it's not an issue for me. Anyway I'm glad to heard that you find my reviews valuable. If you've needs do not hesitate to tell me how I could help you here (as you already know I'm now aware about your cookiecutter project template). |
@zhang-ivy : Were these files used? If so, should we merge this PR, or close it? |
I'm not sure -- I prepped these for @rafwiewiora to put on fah, but I'm guessing they didn't end up making it on there? |
Newly equilibrated structures:
2ajf_ace2
,6m17_ace2
)2ajf_sars
)