Allow tinkerio to read TINKERPATH variable from environment #206
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks for making this PR. Is TINKERPATH the recommended environment variable name from the TINKER distribution or is it a custom name? Is your intention that the environment variable should override what is in the input file? |
|
Of course. I suppose it is a custom name. The only tinker wrapper I am
aware of that use something like this is poltype, which actually uses
TINKERDIR, however I wanted it to be more similar to the
forcebalance variable tinkerpath. The intention is to override the force
balance input with whatever is in the environment.
…On Fri, Mar 12, 2021 at 1:55 PM Lee-Ping Wang ***@***.***> wrote:
Thanks for making this PR. Is TINKERPATH the recommended environment
variable name from the TINKER distribution or is it a custom name? Is your
intention that the environment variable should override what is in the
input file?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#206 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKNB26KNHTZAWPKKAZOPPR3TDJPTTANCNFSM4ZCZ5FOQ>
.
|
|
It would not be an issue if I was only running on one node, however,
distributing jobs requires multiple environments potentially.
…On Fri, Mar 12, 2021 at 2:46 PM Brandon D Walker ***@***.***> wrote:
Of course. I suppose it is a custom name. The only tinker wrapper I am
aware of that use something like this is poltype, which actually uses
TINKERDIR, however I wanted it to be more similar to the
forcebalance variable tinkerpath. The intention is to override the force
balance input with whatever is in the environment.
On Fri, Mar 12, 2021 at 1:55 PM Lee-Ping Wang ***@***.***>
wrote:
> Thanks for making this PR. Is TINKERPATH the recommended environment
> variable name from the TINKER distribution or is it a custom name? Is your
> intention that the environment variable should override what is in the
> input file?
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <#206 (comment)>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AKNB26KNHTZAWPKKAZOPPR3TDJPTTANCNFSM4ZCZ5FOQ>
> .
>
|
|
I think that typically, the environment variable is more "invisible" whereas command line / input file options are more explicitly set by the user. Could you switch the priority order so that the input file will override the environment variable? You can still get the behavior you want by leaving tinkerpath blank in the input file. |
added 2 commits
March 15, 2021 13:28
…ker openmm on GPU), and gas is not in keyfile name, then switch dynamic to dynamicomm (gpu dynamic program). 2) If OPENMM_CUDA_COMPILER is detected in environment, for NPT dynamics, then add N to dynamic command string (program asks for output info from GPU). 3) MUTAL and THOLE keywords are added to lines that include Polarization in them (latest analyze version), so these are excluded from parsing Polarization energy. 4) Temperature is not printed out in latest version (or gpu version), but since it is constant in NPT/NVT, just append the constant temperature instead of parsing. 5) Switch priority of original commit, if tinkerpath keyword is specified, this supersedes any TINKERPATH detected in environment.
…oment and tinkerpath is defined, the tinkerpath variable will supersedes TINKERPATH. If tinkerpath variable is None and TINKERPATH in environment, then this is used instead.
leeping
reviewed
Mar 23, 2021
| def setopts(self, **kwargs): | ||
|
|
||
| """ Called by __init__ ; Set TINKER-specific options. """ | ||
|
|
||
| ## The directory containing TINKER executables (e.g. dynamic) | ||
| if 'tinkerpath' in kwargs: | ||
| self.tinkerpath = kwargs['tinkerpath'] | ||
| if not os.path.exists(os.path.join(self.tinkerpath,"dynamic")): | ||
| if not os.path.exists(os.path.join(self.tinkerpath,"dynamic")) and 'dynamic' not in os.environ.keys(): |
There was a problem hiding this comment.
Could you comment on this? Is "dynamic" supposed to be an environment variable too?
| @@ -910,13 +919,13 @@ def molecular_dynamics(self, nsteps, timestep, temperature=None, pressure=None, | |||
| if temperature is not None: | |||
| md_defs["integrator"] = "stochastic" | |||
| else: | |||
| md_defs["integrator"] = "beeman" | |||
| md_defs["integrator"] = "respa" | |||
There was a problem hiding this comment.
The default parameters are what's used if the user doesn't specify the option in the key file, so it doesn't need to be the most efficient, it only needs to be fail-safe.
Does the use of respa require any additional options in the key file, i.e. specifying the long and short timestep, or the subdivision of the long timestep to make the short timestep?
| md_opts["thermostat"] = None | ||
| # Periodic boundary conditions. | ||
| if self.pbc: | ||
| md_opts["vdw-correction"] = '' | ||
| if temperature is not None and pressure is not None: | ||
| md_defs["integrator"] = "beeman" | ||
| md_defs["integrator"] = "respa" |
| @@ -933,7 +942,7 @@ def molecular_dynamics(self, nsteps, timestep, temperature=None, pressure=None, | |||
|
|
|||
| eq_opts = deepcopy(md_opts) | |||
| if self.pbc and temperature is not None and pressure is not None: | |||
| eq_opts["integrator"] = "beeman" | |||
| eq_opts["integrator"] = "respa" | |||
src/tinkerio.py
Outdated
| self.calltinker("dynamic %s -k %s-eq %i %f %f 4 %f %f" % (self.name, self.name, nequil, timestep, float(nsave*timestep)/1000, | ||
| temperature, pressure), print_to_screen=verbose) | ||
| if 'OPENMM_CUDA_COMPILER' in os.environ.keys(): | ||
| self.calltinker(dynamickeyword+" %s -k %s-eq %i %f %f 4 %f %f N" % (self.name, self.name, nequil, timestep, float(nsave*timestep)/1000,temperature, pressure), print_to_screen=verbose) |
There was a problem hiding this comment.
Please add a comment regarding the extra "N" command line argument
src/tinkerio.py
Outdated
| os.system("rm -f %s.arc" % (self.name)) | ||
|
|
||
| # Run production. | ||
| if verbose: printcool("Running production dynamics", color=0) | ||
| write_key("%s-md.key" % self.name, md_opts, "%s.key" % self.name, md_defs) | ||
| if 'OPENMM_CUDA_COMPILER' in os.environ.keys() and 'gas' not in self.name: |
There was a problem hiding this comment.
This seems to duplicate line 956, could the determination of dynamickeyword be moved "above" the "if nequil>0" block, so it doesn't need to be duplicated?
src/tinkerio.py
Outdated
| if self.pbc and pressure is not None: | ||
| odyn = self.calltinker("dynamic %s -k %s-md %i %f %f 4 %f %f" % (self.name, self.name, nsteps, timestep, float(nsave*timestep/1000), | ||
| temperature, pressure), print_to_screen=verbose) | ||
| if 'OPENMM_CUDA_COMPILER' in os.environ.keys(): |
There was a problem hiding this comment.
Is there some way to simplify this so that it doesn't check for OPENMM_CUDA_COMPILER twice (once when it determines dynamickeyword and again when it decides how to call TINKER?) Maybe you could make the N into a variable "dynamicsuffix" or something like that?
| @@ -996,6 +1019,10 @@ def molecular_dynamics(self, nsteps, timestep, temperature=None, pressure=None, | |||
| havekeys = set() | |||
| first_shot = True | |||
| for ln, line in enumerate(oanl): | |||
| if 'Polarization' in line: | |||
There was a problem hiding this comment.
Can you please comment on why you want to skip parsing these lines out of analyze?
|
In src/tinkerio.py
<#206 (comment)>:
def setopts(self, **kwargs):
""" Called by __init__ ; Set TINKER-specific options. """
## The directory containing TINKER executables (e.g. dynamic)
if 'tinkerpath' in kwargs:
self.tinkerpath = kwargs['tinkerpath']
- if not os.path.exists(os.path.join(self.tinkerpath,"dynamic")):
+ if not os.path.exists(os.path.join(self.tinkerpath,"dynamic"))
and 'dynamic' not in os.environ.keys():
Could you comment on this? Is "dynamic" supposed to be an environment
variable too?
Essentially, I had originally made an environment variable that would allow
dynamic_omm to look like dynamic so this program can run for GPU. I however
have changed this in the lastest commit (its possible I did not remove this
line), however now, there is a dynamickeyword that is dynamic_omm (the GPU
program name) if there is a keyword related to OPENMM/CUDA in the
environment (this keyword is necessary to be able to run).
------------------------------
In src/tinkerio.py
<#206 (comment)>:
@@ -910,13 +919,13 @@ def molecular_dynamics(self, nsteps, timestep,
temperature=None, pressure=None,
if temperature is not None:
md_defs["integrator"] = "stochastic"
else:
- md_defs["integrator"] = "beeman"
+ md_defs["integrator"] = "respa"
The default parameters are what's used if the user doesn't specify the
option in the key file, so it doesn't need to be the most efficient, it
only needs to be fail-safe.
Does the use of respa require any additional options in the key file, i.e.
specifying the long and short timestep, or the subdivision of the long
timestep to make the short timestep?
Yes, I just had to change to respa, since Beeman does not exist for GPU
dynamic_omm. There are no additional options specific to that integrator.
However, I did need to add an extra “N” to a yes/no question for asking to
print out info from GPU for the dynamic_omm program (only if keyword for
OPENMM/CUDA is in environment) .
------------------------------
In src/tinkerio.py
<#206 (comment)>:
md_opts["thermostat"] = None
# Periodic boundary conditions.
if self.pbc:
md_opts["vdw-correction"] = ''
if temperature is not None and pressure is not None:
- md_defs["integrator"] = "beeman"
+ md_defs["integrator"] = "respa"
See above comment.
------------------------------
In src/tinkerio.py
<#206 (comment)>:
@@ -933,7 +942,7 @@ def molecular_dynamics(self, nsteps, timestep,
temperature=None, pressure=None,
eq_opts = deepcopy(md_opts)
if self.pbc and temperature is not None and pressure is not None:
- eq_opts["integrator"] = "beeman"
+ eq_opts["integrator"] = "respa"
See above comment.
------------------------------
In src/tinkerio.py
<#206 (comment)>:
if self.pbc and pressure is not None:
- self.calltinker("dynamic %s -k %s-eq %i %f %f 4 %f %f" % (
self.name, self.name, nequil, timestep, float(nsave*timestep)/1000,
-
temperature, pressure), print_to_screen=verbose)
+ if 'OPENMM_CUDA_COMPILER' in os.environ.keys():
+ self.calltinker(dynamickeyword+" %s -k %s-eq %i %f %f
4 %f %f N" % (self.name, self.name, nequil, timestep,
float(nsave*timestep)/1000,temperature, pressure), print_to_screen=verbose)
Please add a comment regarding the extra "N" command line argument
“However, I did need to add an extra “N” to a yes/no question for asking to
print out info from GPU for the dynamic_omm program (only if keyword for
OPENMM/CUDA is in environment).” Just extra item that dynamic_omm has over
dynamic.
------------------------------
In src/tinkerio.py
<#206 (comment)>:
os.system("rm -f %s.arc" % (self.name))
# Run production.
if verbose: printcool("Running production dynamics", color=0)
write_key("%s-md.key" % self.name, md_opts, "%s.key" % self.name,
md_defs)
+ if 'OPENMM_CUDA_COMPILER' in os.environ.keys() and 'gas' not in
self.name:
This seems to duplicate line 956, could the determination of dynamickeyword
be moved "above" the "if nequil>0" block, so it doesn't need to be
duplicated?
Ah okay some redundancy, but it works 😊
------------------------------
In src/tinkerio.py
<#206 (comment)>:
if self.pbc and pressure is not None:
- odyn = self.calltinker("dynamic %s -k %s-md %i %f %f 4 %f %f"
% (self.name, self.name, nsteps, timestep, float(nsave*timestep/1000),
-
temperature, pressure), print_to_screen=verbose)
+ if 'OPENMM_CUDA_COMPILER' in os.environ.keys():
Is there some way to simplify this so that it doesn't check for
OPENMM_CUDA_COMPILER twice (once when it determines dynamickeyword and
again when it decides how to call TINKER?) Maybe you could make the N into
a variable "dynamicsuffix" or something like that?
Yes this can be made to look “prettier” and more efficient.
------------------------------
In src/tinkerio.py
<#206 (comment)>:
@@ -996,6 +1019,10 @@ def molecular_dynamics(self, nsteps, timestep,
temperature=None, pressure=None,
havekeys = set()
first_shot = True
for ln, line in enumerate(oanl):
+ if 'Polarization' in line:
Can you please comment on why you want to skip parsing these lines out of
analyze?
Basically, new analyze has a line that says Polarization MUTAL or
Polarization THOLE, but program is trying to parse energy from line that
has Polarization energy value. So just preventing this from crashing
program with new analyze.
…On Tue, Mar 23, 2021 at 1:52 PM Lee-Ping Wang ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/tinkerio.py
<#206 (comment)>:
> def setopts(self, **kwargs):
""" Called by __init__ ; Set TINKER-specific options. """
## The directory containing TINKER executables (e.g. dynamic)
if 'tinkerpath' in kwargs:
self.tinkerpath = kwargs['tinkerpath']
- if not os.path.exists(os.path.join(self.tinkerpath,"dynamic")):
+ if not os.path.exists(os.path.join(self.tinkerpath,"dynamic")) and 'dynamic' not in os.environ.keys():
Could you comment on this? Is "dynamic" supposed to be an environment
variable too?
------------------------------
In src/tinkerio.py
<#206 (comment)>:
> @@ -910,13 +919,13 @@ def molecular_dynamics(self, nsteps, timestep, temperature=None, pressure=None,
if temperature is not None:
md_defs["integrator"] = "stochastic"
else:
- md_defs["integrator"] = "beeman"
+ md_defs["integrator"] = "respa"
The default parameters are what's used if the user doesn't specify the
option in the key file, so it doesn't need to be the most efficient, it
only needs to be fail-safe.
Does the use of respa require any additional options in the key file, i.e.
specifying the long and short timestep, or the subdivision of the long
timestep to make the short timestep?
------------------------------
In src/tinkerio.py
<#206 (comment)>:
> md_opts["thermostat"] = None
# Periodic boundary conditions.
if self.pbc:
md_opts["vdw-correction"] = ''
if temperature is not None and pressure is not None:
- md_defs["integrator"] = "beeman"
+ md_defs["integrator"] = "respa"
See above comment.
------------------------------
In src/tinkerio.py
<#206 (comment)>:
> @@ -933,7 +942,7 @@ def molecular_dynamics(self, nsteps, timestep, temperature=None, pressure=None,
eq_opts = deepcopy(md_opts)
if self.pbc and temperature is not None and pressure is not None:
- eq_opts["integrator"] = "beeman"
+ eq_opts["integrator"] = "respa"
See above comment.
------------------------------
In src/tinkerio.py
<#206 (comment)>:
> if self.pbc and pressure is not None:
- self.calltinker("dynamic %s -k %s-eq %i %f %f 4 %f %f" % (self.name, self.name, nequil, timestep, float(nsave*timestep)/1000,
- temperature, pressure), print_to_screen=verbose)
+ if 'OPENMM_CUDA_COMPILER' in os.environ.keys():
+ self.calltinker(dynamickeyword+" %s -k %s-eq %i %f %f 4 %f %f N" % (self.name, self.name, nequil, timestep, float(nsave*timestep)/1000,temperature, pressure), print_to_screen=verbose)
Please add a comment regarding the extra "N" command line argument
------------------------------
In src/tinkerio.py
<#206 (comment)>:
> os.system("rm -f %s.arc" % (self.name))
# Run production.
if verbose: printcool("Running production dynamics", color=0)
write_key("%s-md.key" % self.name, md_opts, "%s.key" % self.name, md_defs)
+ if 'OPENMM_CUDA_COMPILER' in os.environ.keys() and 'gas' not in self.name:
This seems to duplicate line 956, could the determination of
dynamickeyword be moved "above" the "if nequil>0" block, so it doesn't need
to be duplicated?
------------------------------
In src/tinkerio.py
<#206 (comment)>:
> if self.pbc and pressure is not None:
- odyn = self.calltinker("dynamic %s -k %s-md %i %f %f 4 %f %f" % (self.name, self.name, nsteps, timestep, float(nsave*timestep/1000),
- temperature, pressure), print_to_screen=verbose)
+ if 'OPENMM_CUDA_COMPILER' in os.environ.keys():
Is there some way to simplify this so that it doesn't check for
OPENMM_CUDA_COMPILER twice (once when it determines dynamickeyword and
again when it decides how to call TINKER?) Maybe you could make the N into
a variable "dynamicsuffix" or something like that?
------------------------------
In src/tinkerio.py
<#206 (comment)>:
> @@ -996,6 +1019,10 @@ def molecular_dynamics(self, nsteps, timestep, temperature=None, pressure=None,
havekeys = set()
first_shot = True
for ln, line in enumerate(oanl):
+ if 'Polarization' in line:
Can you please comment on why you want to skip parsing these lines out of
analyze?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#206 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AKNB26LMYB2TTUTXTFO4WQ3TFDPODANCNFSM4ZCZ5FOQ>
.
|
added 21 commits
May 6, 2021 14:01
…or tinker GPU dynamics 2) Need to overide masterhost TINKERPATH input with enviorment of node since OS /libraries for installation can be different for worker node and master host.
…otherwise it will crash.
…cebalance data.csv file.
…s not longer used. Set gpu dynamics to dynamic_gpu and search for GPUDYNAMICS keyword in envioronment to switch to GPU dynamics.
…al data as inputs into wrapper.
…e temp/pressure points. Read input csv example with experimental data.
…han two poltype jobs as input for force balance.
…eady has), then make sure to not add to input file for ForceBalance.
…ile, just turn off liquid sim for those points and dont add to forcebalance input file.
Bug fix, changing xyz type numbers for multiple molecules.
Make sure no absolute paths go into WorkQueue, it will throw fatal error.
added 3 commits
February 21, 2022 15:25
|
@bdw2292 I'm reviewing all the open PRs and this one is looking pretty huge at this point, and it contains lots of code. I do want to include your features into the master branch, but I'm not sure I can understand all your modifications on my own. Could we schedule a meeting sometime to go over what you wrote? You may feel free to ask your PI to join as well. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.