Add apo ACE2 and SARS RBD:ACE2 structures - #31
Conversation
4383
left a comment
4383
left a comment
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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-structuresMaybe 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? |
4 participants
Newly equilibrated structures:
2ajf_ace2,6m17_ace2)2ajf_sars)