Conversation
|
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
|
No API break detected ✅ |
IAlibay
left a comment
IAlibay
left a comment
There was a problem hiding this comment.
Overall this looks good to me. There's a few areas where it might be good to enhance things, especially when it comes maybe just sticking on the checkpoint reporter the whole way through the simulation, but they wouldn't affect the overall organization of the code.
| return pdbs | ||
|
|
||
|
|
||
| class PlainMDProtocol(gufe.Protocol): |
There was a problem hiding this comment.
[scope] It may be good to take the opportunity to move Protocols, results, settings and units to their own files.
| equilibration_length=1.0 * unit.nanosecond, | ||
| production_length=5.0 * unit.nanosecond, | ||
| ), | ||
| output_settings=MDOutputSettings(), |
There was a problem hiding this comment.
Is changing the filename necessary or just a nice-to-have?
| Exception if anything failed | ||
| """ | ||
|
|
||
| if verbose: |
There was a problem hiding this comment.
[scope] It would be nice to try to split up some of the code here into their own class methods. E.g. _prepare, _get_settings, etc.. like we do in the AFE and RFE Protocols).
We're not in a situation where we can deduplicate across Protocols yet, however attempting to do so in each Protocol would be useful in finding areas of duplication.
| integrator_settings = protocol_settings.integrator_settings | ||
|
|
||
| # is the timestep good for the mass? | ||
| settings_validation.validate_timestep(forcefield_settings.hydrogen_mass, timestep) |
There was a problem hiding this comment.
[scope] we should add a _validate method to the Protocol.
| # g. Save the system and positions to file | ||
| system_outfile = shared_basepath / "system.xml.bz2" | ||
| serialization.serialize(stateA_system, system_outfile) | ||
| # not really need if we save out the pre-minimized file |
There was a problem hiding this comment.
I somewhat agree, however PDBs are low precision, savin the npy is good in my opinion.
| traj.save_pdb(shared_basepath / output_settings.minimized_structure) | ||
| # equilibrate | ||
| # NVT equilibration | ||
| if equil_steps_nvt: |
There was a problem hiding this comment.
[scope] Thinking about this - is there any reason we can't just add the checkpoint reporter at this stage? It's more likely that someone running the PlainMD protocol would be running longer equilibrations / productions, so it might be nice to be able to restart at any point during it.
| """ | ||
| if verbose: | ||
| self.logger.info("Creating system") | ||
| if shared_basepath is None: |
There was a problem hiding this comment.
Because the paths can be None, I think you'll want to have logic to deal with this somewhere here.
| stateA_topology, stateA_positions, file=f, keepIds=True | ||
| ) | ||
|
|
||
| # 10. Get platform |
There was a problem hiding this comment.
[nit] The comments will need to be updated once the structure is settled.
| if x.getName() == "MonteCarloBarostat": | ||
| x.setFrequency(0) | ||
|
|
||
| simulation.context.setVelocitiesToTemperature(to_openmm(temperature)) |
There was a problem hiding this comment.
[scope / nit] it would definitely be good to split this up into method calls or something like that. There's a lot of code in an "else" section.
2 participants
Fixes #1719, #1720, #1721
This PR splits up the plain MD protocol unit into a setup and simulation unit and adds the ability to restart failed simulations.
Notes
Checklist
newsentry, or the changes are not user-facing.pre-commit.ci autofix.Manual Tests: these are slow so don't need to be run every commit, only before merging and when relevant changes are made (generally at reviewer-discretion).
Developers certificate of origin