openforcefield / smarty

Chemical perception tree automated exploration tool.
http://openforcefield.org
MIT License
19 stars 8 forks source link

[WIP] updating SMIRKS parsing in ChemicalEnvironments #249

Closed bannanc closed 7 years ago

bannanc commented 7 years ago

This pull request primarily addresses issue #248

I'm still working on testing changes and they aren't quite ready, but I wanted to get this open just to have a record of what I have worked on so far. I'll add some more information tomorrow including updating examples (here and in openforcefield repo) to show how Chemical Environments can be used.

It also adds an isValid() method to Chemical Environments as discussed in issue #96

jchodera commented 7 years ago

We've migrated environment.py out of this repo and into https://github.com/open-forcefield-group/openforcefield as https://github.com/open-forcefield-group/openforcefield/tree/master/openforcefield/typing/chemistry

We should probably update that version instead and migrate the smarty code to use that version, unless both are still rapidly changing at the moment.

davidlmobley commented 7 years ago

Yes, @bannanc - can you make the modifications to chemical environments on the openforcefield repo and instead make a PR here that will use the chemical environments from openforcefield?

And, @jchodera - do we have a conda-installable version yet?

@bannanc - one issue will be that this makes the process of "implementing new code on openforcefield" and then "using new code from openforcefield in smarty" somewhat more involved, in that to make updates to openforcefield conda-installable, we need to do as follows, I believe:

jchodera commented 7 years ago

And, @jchodera - do we have a conda-installable version yet?

It's been conda-installable for 11 days: https://anaconda.org/omnia/openforcefield/files

You can also install it via pip if you want to grab the development version:

pip install git+https://github.com/open-forcefield-group/openforcefield.git
jchodera commented 7 years ago

We could have the smarty travis install the latest development version via

pip install git+https://github.com/open-forcefield-group/openforcefield.git

if things are going to be changing rapidly within openforcefield for a while.

davidlmobley commented 7 years ago

Ah, that's a good idea, @jchodera . Note the above, @bannanc .

bannanc commented 7 years ago

@jchodera and @davidlmobley Yes, I know we've moved this, I wrote the issue here initially thinking it might be a problem with smirky, but it seems to be a problem with how the chemical environments were writing out replacement strings. Testing that lead to a few other types of SMIRKS strings that the environment couldn't parse correctly. I started an issue on that repo as well.

I will use the open pull request to make the slight modification I have to smirky and make it so smarty and smirky import from the openforcefield modules.

I'll add the updated environments to my WIP pull request in openforcefield.

bannanc commented 7 years ago

I'm getting closer to solving the parsing issues, I'm working on writing some "extra weird" SMIRKS strings to test it.

Once I'm convinces the parsing works, I'll move everything to openforcefield and make sure the smarty/smirky scripts use openforcefield.

bannanc commented 7 years ago

I thought I was getting closer, but given the way tests are failing, I've clearly broke a few things also...

bannanc commented 7 years ago

Updated environments are now available in the openforcefile repo.

I have changed this PR to be about switch smarty to be dependent on openforcefield, but I think the discussion here is confusing so I am going to close this and open a new PR for removing openforcefield redundancy.