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

Add apo ACE2 and SARS RBD:ACE2 structures #31

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

zhang-ivy
Copy link
Contributor

Newly equilibrated structures:

  • Apo ACE2 (2ajf_ace2, 6m17_ace2)
  • SARS RBD:ACE2 (2ajf_sars)

Copy link
Contributor

@4383 4383 left a 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:
Copy link
Contributor

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:
Copy link
Contributor

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:
Copy link
Contributor

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:
Copy link
Contributor

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:
Copy link
Contributor

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:
Copy link
Contributor

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:
Copy link
Contributor

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:
Copy link
Contributor

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:
Copy link
Contributor

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:
Copy link
Contributor

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

@jaimergp
Copy link
Contributor

jaimergp commented Apr 9, 2020

Instead of os.path.join, why not pathlib.Path?

@4383
Copy link
Contributor

4383 commented Apr 9, 2020

Instead of os.path.join, why not pathlib.Path?

Why not... as long we surround path construct with something I'm happy :)

@4383
Copy link
Contributor

4383 commented Apr 9, 2020

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.

@jchodera
Copy link
Member

@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!

@4383
Copy link
Contributor

4383 commented Apr 10, 2020

@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).

@jchodera
Copy link
Member

@zhang-ivy : Were these files used? If so, should we merge this PR, or close it?

@zhang-ivy
Copy link
Contributor Author

zhang-ivy commented Oct 27, 2020

@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?

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 this pull request may close these issues.

None yet

4 participants