Fix to environment SMIRKS parsing and update README and detailed examples with environment #7
Conversation
bannanc
changed the title
|
I'm getting closer to solving the parsing issues, I'm working on writing some "extra weird" SMIRKS strings to test it. |
|
@davidlmobley I think this is ready for a code review whenever you have time. |
bannanc
changed the title
There was a problem hiding this comment.
@bannanc - looks good to me except minor typos/clarifications noted, and a question about whether you're testing some of the functionality which has been corrected/adjusted (if not then you probably want to modify tests slightly to do so).
Should be good to merge once these minor issues are corrected. Nice work!
We should be sure to do a new point release once this is merged.
README.md
Outdated
| Documentation forthcoming. | ||
| See `examples/chemicalEnvironments/` for now. | ||
| ChemicalEnvironments are a python class used to parse and manipulate SMIRKS strings. | ||
| They were created with the goal of being able to automatically and randomly generate parameters for SMIRKS Native-Open Force Fields. |
There was a problem hiding this comment.
They were created with the goal of being able to automatically and randomly generate parameters for SMIRKS Native-Open Force Fields.
Is this the right way of explaining this? Currently we don't do anything with parameters. I think maybe it would be more accurate to say that they were created with the goal of being able to automatically and randomly explore chemical perception, so that someday we can sample over chemical perception at the same time as developing parameters... ?
| ### Initiating ChemicalEnvironments | ||
|
|
||
| All Chemical Environments can be initated using SMIRKS strings. | ||
| The general ChemicalEnvironment, with no input string is completely empty, however there |
There was a problem hiding this comment.
"General ChemicalEnvironment objects with no input string are completely empty. However, there are five..." (The current wording "...with no input string is completely empty..." is confusing).
There was a problem hiding this comment.
Also, singlequote names of data structures if possible.
There was a problem hiding this comment.
(For example, AtomChemicalEnvironment not AtomChemicalEnvironment -- it makes it more clear you're talking about code.)
|
|
||
| * `moves_using_environments.ipynb` - uses the `moveTrees.uniq.*.txt` files to illustrate how to make moves in chemical perception space. | ||
|
|
||
| For a more thorough and automated tool for using ChemicalEnvironments to make changes to chemical perception see [smirky](https://github.com/open-forcefield-group/smarty), a tool we developed to rediscover SMIRNOFF parameters. |
There was a problem hiding this comment.
Are there features of SMARTS/SMIRKS which are NOT (yet) supported which should be noted for the record? For example, a "Not yet supported" section?
|
|
||
| ### ANDtypes and ORtypes | ||
|
|
||
| ChemicalEnvironments store information about the SMIRKS string in the same way a |
There was a problem hiding this comment.
Is this backwards? It's not that they store information about a SMIRKS string -- they store information about a chemical graph and provide a mechanism to translate to and from SMIRKS patterns, right?
| print("New Atom 1: %s " % atom1.asSMIRKS()) | ||
| # New Atom 1: [#6X3,#7X2;+0:1] | ||
|
|
||
| # Change atom2's AND and OR types with the add*type methods |
There was a problem hiding this comment.
Maybe you want a comment here to remind people what WAS in this? For example, "#Remember, angle contains the environment corresponding to SMIRKS smirks = "[#6X3,#7;+0:1]~;@[#8;r:2]~;@[#6X3,#7;+0:3]""
There was a problem hiding this comment.
(It took me a minute to figure out I had to scroll up to find out what this was talking about).
| beta_list = angle.getBetaAtoms | ||
| # Note - Atoms can be replaced with Bonds in each of the above examples | ||
| ``` | ||
| 3. Report the minimum order of a bond with Bond.getOrder |
There was a problem hiding this comment.
"minimum order"? Not "order"? Confuses me.
There was a problem hiding this comment.
I'll add more commentary, but this is because there could be more than one bond type. For example, [*]-,:,=[*] could be single, aromatic, or double. getOrder() on that bond would return 1.
| atomB = angle.selectAtom(B) | ||
| bondAB = angle.getBond(atomA, atomB) # returns None if no bond found | ||
| ``` | ||
| 6. Get atoms bound to a specified atom with getNieghbors |
There was a problem hiding this comment.
getNeighbors misspelled
|
|
||
| ### `using_environments.ipynb` | ||
|
|
||
| This notebook should be your first stop for interactively understanding and using ChemicalEnvironemnts. |
There was a problem hiding this comment.
(Note that some of these typos -- though not this one -- also show up in your using_environments.ipynb file and should be corrected there too.)
|
|
||
| # Store the smirks pattern before and after the relevant atom | ||
| pre_smirks = smirks[:a_in] | ||
| post_smirks = smirks[a_out+1:] | ||
|
|
||
| # Check for ring index, i.e. the 1s in "[#6:1]1-CCCCC1" |
There was a problem hiding this comment.
Were we not handling ring subsearches like this before? If not we probably want to note this prominently in listing the changes this PR brings in. (Basically just we want to make it easy to tell where/when we made any especially important/big changes.)
There was a problem hiding this comment.
(Also, have you modified tests to test this?)
There was a problem hiding this comment.
No we weren't covering them before, I wasn't aware that you could use the ring indexing until fairly recently.
I haven't added them to the tests yet, I have the complicated list in the ipython notebook. I could add that to the list in the tests.
| of pattern is not parseable into ChemicalEnvironments" % smirks) | ||
| if smirks.find('>') != -1: | ||
| raise SMIRKSParsingError("Error: Provided SMIRKS ('%s') \ | ||
| contains a '>' indicating a reaction. This type of pattern is not parseable \ |
There was a problem hiding this comment.
Nice. Probably want to get this into a README somewhere too. :)
|
@davidlmobley I made the changes you pointed out, I also updated the environment tests to make sure it could parse ring SMIRKS. |
|
Thanks, @bannanc . So you should be good to merge once tests pass (currently broken). |
|
@davidlmobley |
|
Thanks, @bannanc ! |
Fix to environment SMIRKS parsing and update README and detailed examples with environment
My goal with this pull request is to get the examples to run correctly, specifically I will:
'.'and'>>'in SMIRKS patterns and raise error