Update for OEAssignCharges #255

nathanmlim · 2017-05-09T03:49:30Z

Addressing issue #252 by replacing the line to call the OEAssignCharges from OEToolkits version 2017.02.01. Added some warning messages as well.

davidlmobley · 2017-05-09T03:52:17Z

@nathanmlim : Looks good to me, except could you also address the other aspect of #252 at the same time? Specifically:

HOWEVER, there is one extension of the charging protocol that he recommends. Specifically, the normal (small) RMSD threshold for omega is still recommended as long as the maximum number of conformers (maxconfs) is not reached. But when maxconfs is hit, what will happen is that omega will begin to prune the least favorable conformers in order to maintain only maxconfs; with a low RMSD threshold this can result in a gradual loss of diversity of the conformers and thus correspondingly worse performance for the charge calculations.

Thus, Chris's recommendation was this: Whenever maxconfs is hit, increase the RMSD threshold for conformer generation until maxconfs is no longer hit. This will ensure that conformers are diverse for very flexible molecules, while still using the recommended low RMSD threshold for relatively rigid molecules.

Or, if you'd prefer not to, then we need to NOT close the issue when this is resolved and just rename it/clarify what still needs to be done. The changes involved for this are obviously more involved. So, let me know what you prefer.

nathanmlim · 2017-05-09T04:09:16Z

Travis tests appear to be failing due to inactivity?
https://travis-ci.org/choderalab/openmoltools/jobs/230204889#L2143

davidlmobley · 2017-05-09T04:11:46Z

That's odd. On occasion we've seen this if there is no output for a while. I think there is a setting to adjust how long it takes for the tests to time out? Or, adjust the verbosity level so it will produce more output?

Have you run tests locally?

andrrizzi · 2017-05-09T12:56:05Z

openmoltools/openeye.py


    #Determine conformations to return
    if keep_confs == None:
+        logger.warning(
+            "keep_confs was set to None. Returned molecules will not be charged.")


I think in this case the returned molecule is still charged, but the coordinates are reset to those of the passed molecules, or did something changed with the new OE?

Do you think it'll be ok to send these messages to logger.debug instead of warning? Otherwise it will be very hard to silent them in our applications.

@nathanmlim - Andrea is right, unless you've changed it the warning message here is wrong. The behavior we'd implemented (as per the doc string) was that if keep_confs = None then the computed charges are applied to the provided conformation of the molecule, rather than any of the generated conformations.

@andrrizzi Thanks for catching that. I'll switch it to debug as well.

nathanmlim · 2017-05-09T15:42:15Z

Travis tests are stalling
https://travis-ci.org/choderalab/openmoltools/jobs/230387708#L2166

Test generation of single ffxml file from a list of molecules ... 
No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received

Any suggestions on what to do here? @davidlmobley @andrrizzi

davidlmobley · 2017-05-09T16:24:23Z

@andrrizzi - do you have advice on the "stalling tests" issue?

andrrizzi · 2017-05-09T16:39:25Z

That's a weird one. Is it possible that simply the new charge assigning scheme takes much longer than the old one? That test ends up calling get_charges through generateForceFieldFromMolecules, so this could be a possibility. Or maybe it could be a problem with the license that makes the code hanging?

In both cases, I'd make an experiment and temporarily revert to the old charge engine to see if it's a problem related to that or not.

Is this something you can reproduce locally?

nathanmlim · 2017-05-09T22:58:43Z

So it looks like it's stalling because the new charging engine takes longer. Is there a way to make it so travis doesn't time out?

andrrizzi · 2017-05-10T13:44:37Z

Hmm. Maybe you could yield a partial function for each test case instead of having a single test running them all. This should print something as soon as the first one passes and continue. 10mins is a long time for a test though. You could cut superfluous test cases. Or maybe we should let the user decide which engine to use through an argument of openeye.get_charges(..., engine)? If so we could use the old engine for testing.

davidlmobley · 2017-05-10T15:41:36Z

@andrrizzi @nathanmlim - is this the test where we charge a whole bunch of molecules? I recall wondering in the past why we were testing so many molecules. I don't see any reason to test charging many, many molecules.

davidlmobley · 2017-05-10T15:42:29Z

It's also not a bad idea to make the engine be an option, with the default to the new engine. Then you could just, say, test on charging a single molecule (?) with the new engine and another molecule or two with the old engine...?

andrrizzi · 2017-05-10T21:06:08Z

I think this is the problematic test. I think it calls get_charges on these 5 molecules through generateForceFieldFromMolecules().

The speed loss is actually quite significant. The same test took 22 second to complete with the old engine, so other tests could be affected by this.

nathanmlim · 2017-05-10T21:18:07Z

I've added a legacy option to the get_charges function. It will default to use the old engine due to the speed loss, so tests should continue using the old engine and users can opt to use the new one if they like.

davidlmobley · 2017-05-10T21:28:23Z

Sounds reasonable, @nathanmlim, but can you make sure the new charging option gets used in at least one test? We don't want to have it going untested.

I can see it being significantly slower, as I believe it's doing more careful selection of conformed for charging and more charge calculations for averaging in certain cases. So there will be more cost, but also better charges resulting.

nathanmlim · 2017-05-11T00:01:16Z

Sounds reasonable, @nathanmlim, but can you make sure the new charging option gets used in at least one test? We don't want to have it going untested.

@davidlmobley Valid point. BTW, do you know if there is a way to check the installed version of OEToolKits? That would be another way to make sure it ends up using the legacy charging engine.

davidlmobley · 2017-05-11T00:07:52Z

There is a way, but I don't offhand remember what it is. However, I'm fairly certain I have code in this repo which does so, somewhere, as at some point there were certain things we required a specific version to be able to do.

…

On Wed, May 10, 2017, 5:01 PM Nathan Lim ***@***.***> wrote: Sounds reasonable, @nathanmlim <https://github.com/nathanmlim>, but can you make sure the new charging option gets used in at least one test? We don't want to have it going untested. @davidlmobley <https://github.com/davidlmobley> Valid point. BTW, do you know if there is a way to check the installed version of OEToolKits? That would be another way to make sure it ends up using the legacy charging engine. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#255 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGzUYX4LhycMeCP_Qtmxh7aIhBPCf9Ntks5r4k_NgaJpZM4NUyOv> .

nathanmlim · 2017-05-11T01:44:21Z

Ah found it in .travis.yml

python -c "import openeye; print(openeye.version)"

nathanmlim · 2017-05-17T16:08:00Z

@davidlmobley @andrrizzi

Addition of tests are linked here

Okay to merge?

davidlmobley

Thanks, @nathanmlim . This looks excellent. Approving and will merge. Shall we do a release to incorporate this so we can push it out to Omnia?

nathanmlim · 2017-05-17T16:35:37Z

Thanks, @nathanmlim . This looks excellent. Approving and will merge. Shall we do a release to incorporate this so we can push it out to Omnia?

Not a high priority right now but will eventually need it on a conda channel roughly mid-June. Thanks!

Nathan M. Lim added 2 commits May 8, 2017 11:29

Replace OEAssignPartialCharges with OEAssignCharges

d1cc7e8

Add warning messages for keep_confs settings

b65cd51

nathanmlim requested a review from davidlmobley May 9, 2017 03:49

davidlmobley approved these changes May 9, 2017

View reviewed changes

andrrizzi reviewed May 9, 2017

View reviewed changes

Change logger warnings to debug, increase travis verbosity to 3

ca8164f

Revert back changes to see if travis tests pass

2680d38

Nathan M. Lim added 2 commits May 9, 2017 23:16

Switch to OEAssignCharges and add 'travis_wait' to tests

ab60ca5

Move travis_wait to install line

68e43b9

Add option to use new OE charging engine, defaults to old function

ce4e9db

Remove travis_wait line

837a859

nathanmlim changed the title ~~Update for OEAssignCharges~~ [WIP] Update for OEAssignCharges May 10, 2017

Add tests for new OEAssignCharges function.

68c19a6

nathanmlim changed the title ~~[WIP] Update for OEAssignCharges~~ Update for OEAssignCharges May 17, 2017

davidlmobley approved these changes May 17, 2017

View reviewed changes

davidlmobley merged commit 77a71ac into master May 17, 2017

davidlmobley deleted the update/assigncharges branch May 17, 2017 16:31

davidlmobley mentioned this pull request May 19, 2017

Do a new release #257

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update for OEAssignCharges #255

Update for OEAssignCharges #255

nathanmlim commented May 9, 2017

davidlmobley commented May 9, 2017

nathanmlim commented May 9, 2017

davidlmobley commented May 9, 2017

andrrizzi May 9, 2017

davidlmobley May 9, 2017

nathanmlim May 9, 2017

nathanmlim commented May 9, 2017 •

edited

Loading

davidlmobley commented May 9, 2017

andrrizzi commented May 9, 2017

nathanmlim commented May 9, 2017

andrrizzi commented May 10, 2017

davidlmobley commented May 10, 2017

davidlmobley commented May 10, 2017

andrrizzi commented May 10, 2017

nathanmlim commented May 10, 2017

davidlmobley commented May 10, 2017

nathanmlim commented May 11, 2017

davidlmobley commented May 11, 2017 via email

nathanmlim commented May 11, 2017

nathanmlim commented May 17, 2017

davidlmobley left a comment

nathanmlim commented May 17, 2017

Update for OEAssignCharges #255

Update for OEAssignCharges #255

Conversation

nathanmlim commented May 9, 2017

davidlmobley commented May 9, 2017

nathanmlim commented May 9, 2017

davidlmobley commented May 9, 2017

andrrizzi May 9, 2017

Choose a reason for hiding this comment

davidlmobley May 9, 2017

Choose a reason for hiding this comment

nathanmlim May 9, 2017

Choose a reason for hiding this comment

nathanmlim commented May 9, 2017 • edited Loading

davidlmobley commented May 9, 2017

andrrizzi commented May 9, 2017

nathanmlim commented May 9, 2017

andrrizzi commented May 10, 2017

davidlmobley commented May 10, 2017

davidlmobley commented May 10, 2017

andrrizzi commented May 10, 2017

nathanmlim commented May 10, 2017

davidlmobley commented May 10, 2017

nathanmlim commented May 11, 2017

davidlmobley commented May 11, 2017 via email

nathanmlim commented May 11, 2017

nathanmlim commented May 17, 2017

davidlmobley left a comment

Choose a reason for hiding this comment

nathanmlim commented May 17, 2017

nathanmlim commented May 9, 2017 •

edited

Loading