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

Several small fixes #281

Merged
merged 3 commits into from
Feb 4, 2023
Merged

Several small fixes #281

merged 3 commits into from
Feb 4, 2023

Conversation

leeping
Copy link
Owner

@leeping leeping commented Jan 26, 2023

In forcefield.py:

  • Fix a .ffxml parsing bug in which leading whitespace causes a floating point number to not be recognized.
  • The OpenFF unit string is incompatible with evaluated parameters (evaluated parameters are not independently optimized, but are functions of other parameters). As a temporary fix, units have been disabled for evaluated parameters. @j-wags : Your input could be helpful here, would OpenFF ever want evaluated parameters with units?
  • Updated the OpenMM interface to incorporate the new method for obtaining names of Force objects while preserving compatibility with OpenMM <7.6.
  • Fixed CopyAmoebaVdwParameters such that type parameters and type pair parameters are copied.
  • Added the ability to replace parameter tag names with more descriptive ones when building parameter names (such as <Angle .../> in AmoebaOutOfPlaneBendForce

In objective.py:

  • Fixed remove micro-iteration folder feature. The previous code never worked.

In studies:

  • Changed iAMOEBA hydrogen vdW parameter sigma to 1.0 (for compatibility with OpenMM >= 7.6).

TODO:

  • Undo unintended change to make-vdata.py.

@pavankum
Copy link
Contributor

Fixed remove micro-iteration folder feature. The previous code never worked.

It never worked for me either, Trevor suggested this workaround to me using glob to get the folders and this works, it goes here if this feature is still of interest to anyone, https://github.com/leeping/forcebalance/blob/master/src/optimizer.py#L889

            if not self.retain_micro_outputs:
                for microo in glob.glob("**/micro_*", recursive=True):
                    shutil.rmtree(microo)

@pavankum
Copy link
Contributor

Ahh sorry, I see that you already made required changes, please ignore the previous comment.

Copy link
Collaborator

@j-wags j-wags left a comment

Choose a reason for hiding this comment

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

This should be good to merge if you're OK with the amoeba test beginning to fail (not sure about all the context there so it's your call)

  • (not blocking, commented in this review) One stray debug statement left over.
  • (your call) The CI is showing a new error: FAILED src/tests/test_engine.py::TestAmoebaWater6::test_interaction_energy - IndexError: list index out of range

The OpenFF unit string is incompatible with evaluated parameters (evaluated parameters are not independently optimized, but are functions of other parameters). As a temporary fix, units have been disabled for evaluated parameters. @j-wags : Your input could be helpful here, would OpenFF ever want evaluated parameters with units?

"Ever" is a big word, but if it hasn't been working since unit-bearing values in SMIRNOFF started being written as full quantities (around 2019?) then I think we're good without it.

src/openmmio.py Outdated Show resolved Hide resolved
Co-authored-by: Jeff Wagner <jwagnerjpl@gmail.com>
@leeping leeping merged commit fb20fb4 into master Feb 4, 2023
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