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

Repex submission new #222

Closed
wants to merge 14 commits into from
Closed

Repex submission new #222

wants to merge 14 commits into from

Conversation

candidechamp
Copy link
Collaborator

Description

This pull request includes the addition of code to submit replica exchange (Temperature, Hamiltonian, and RE-EDS) simulations using the exact same convention as for normal simulations.

In short the call to the code in schedule_MD_job.py written by the simulation scheduler will automtaically adjust everything it needs to dispatch the calculation to either md, md_mpi or repex_mpi.

The waiting for output files was adjusted to wait for all N (number of replicas) output cnfs. Similarly the path provided is slightly adjusted to match the convention of gromos for replica exchange depending on the presence / absence of the CONT keyword.

The code was also tested with regular MD simulations, and there was no problem in submitting / zipping files for those.

@MTLehner @pultar As you might be the most frequent users of pyGromos at the moment you might want to checkout my branch and see if the submission for your systems still works fine!

Todos

  • Testing by other people

Status

  • Ready to go

@pultar
Copy link
Collaborator

pultar commented Feb 24, 2022

Will check it out later!

@codecov
Copy link

codecov bot commented Feb 24, 2022

Codecov Report

Merging #222 (8a697bf) into release3 (80ba1ed) will decrease coverage by 0.09%.
The diff coverage is 19.54%.

Impacted file tree graph

@@             Coverage Diff              @@
##           release3     #222      +/-   ##
============================================
- Coverage     54.63%   54.54%   -0.10%     
============================================
  Files            92       92              
  Lines         13884    13902      +18     
============================================
- Hits           7586     7583       -3     
- Misses         6298     6319      +21     
Flag Coverage Δ
unittests 54.54% <19.54%> (-0.10%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...orkers/simulation_workers/simulation_run_worker.py 10.41% <2.27%> (-2.80%) ⬇️
pygromos/gromos/gromosXX.py 13.51% <11.11%> (-0.03%) ⬇️
pygromos/files/blocks/imd_blocks.py 79.04% <54.16%> (+2.16%) ⬆️
pygromos/files/simulation_parameters/imd.py 64.23% <100.00%> (-0.77%) ⬇️
pygromos/utils/utils.py 75.22% <0.00%> (+2.65%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 80ba1ed...8a697bf. Read the comment docs.

setattr(self, "NRET", int(content[3].split()[0]))
T_values = list(map(float, content[5].split()))
if(len(T_values)== self.NRET):
setattr(self, "RET", T_values)
else:
raise IOError("REPLICA: NRET was not equal to the number of Temperatures (RET) in IMD!")
setattr(self, "LRESCALE", int(content[7].split()[0]))
print (content[7])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a debug print?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes forgot to remove this one

@pultar pultar self-requested a review February 24, 2022 21:11
Candide Champion added 2 commits February 25, 2022 10:22
Copy link
Collaborator

@pultar pultar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My local tests run as well (I don't do much REPEX). However, I am against the change:

initialize_first_run= True

I believe it makes PyGromos more unpredictable.

@@ -21,7 +21,7 @@ def do(in_simSystem: Gromos_System,
simulation_run_num: int,
equilibration_run_num: int = 0,
work_dir: str = None,
initialize_first_run= False, reinitialize_every_run= False,
initialize_first_run= True, reinitialize_every_run= False,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm against this change. I believe PyGromos should by default not change anything.

@candidechamp
Copy link
Collaborator Author

@pultar Very good to hear that your simulations are unaffected and still run well with these changes.

Concerning the initialize_first_run variable. I would tend to say that the value should be True by default (as you typically don't have a cnf which already has velocities when you want to run your first MD).

But I don't have particularly strong feelings about it either. I just know that one (and maybe more) of the downstream functions such as those in pygromos/simulations/hpc_queuing/job_scheduling/workers/simulation_workers/simulation_run_worker.py has the default set to True.

As long as all functions with they argument have the same default then I am happy.

@candidechamp
Copy link
Collaborator Author

note: I only fixed some merge conflicts in this previous merge. I still have a few more things to do on this branch so it should not be merged just yet.

@candidechamp
Copy link
Collaborator Author

candidechamp commented Apr 11, 2022

I am closing the pull request as I re-implemented these changes (with slight improvements in a different branch, see #288)

@candidechamp candidechamp deleted the repex_submission_new branch April 19, 2022 08:52
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.

3 participants