Add SMARTS selection by cbouy · Pull Request #2883 · MDAnalysis/mdanalysis

cbouy · 2020-07-31T15:46:57Z

Part of #2468
Depends on #2775

Changes made in this Pull Request:

Adds atom selection based on SMARTS queries: u.select_atoms("[C;R;!a]", smarts=True)

An example with Proline:

PR Checklist

Tests?
Docs?
CHANGELOG updated?
Issue raised/referenced?

@richardjgowers @IAlibay @fiona-naughton

pep8speaks · 2020-07-31T15:47:03Z

Hello @cbouy! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-08-26 16:53:09 UTC

richardjgowers

Would it be possible to have a smarts keyword instead of forcing one type or another of selection? Eg "smarts [#7] and not resname ASP"

cbouy · 2020-07-31T16:46:49Z

Since some SMARTS have parenthesis in them I'd need to find a way to bypass the "split original selection string on spaces and parenthesis" thing, but should be doable

cbouy · 2020-07-31T18:04:56Z

Done!

richardjgowers · 2020-07-31T18:16:42Z

This is pretty awesome btw :)

richardjgowers · 2020-07-31T18:15:15Z

package/MDAnalysis/core/selection.py

+        if not pattern:
+            raise ValueError("{!r} is not a valid SMARTS query".format(
+                self.pattern))
+        mol = group.convert_to("RDKIT")


Might be nice to try and cache this in Topology.

The mol (topology, not coordinates) is already cached in the converter so that we don't need to rebuild it for every frame of a trajectory:
https://github.com/cbouy/mdanalysis/blob/rdkit-converter/package/MDAnalysis/coordinates/RDKit.py#L253-L264

richardjgowers · 2020-07-31T18:15:50Z

package/MDAnalysis/core/selection.py

+        mol = group.convert_to("RDKIT")
+        matches = mol.GetSubstructMatches(pattern, useChirality=True)
+        indices = [idx for match in matches for idx in match]
+        indices = [mol.GetAtomWithIdx(i).GetIntProp("_MDAnalysis_index")


Can we not predict what idx RDKit will assign? Ie do the atoms sometimes get reordered?

They get reordered when using reactions to "standardize" some functional groups like nitro, sulfone...etc into the correct form. I could also force reordering atoms at the end of the conversion but that requires building a new molecule again I think.

richardjgowers · 2020-07-31T18:38:37Z

package/MDAnalysis/core/selection.py

+    token = 'smarts'
+
+    def __init__(self, parser, tokens):
+        pattern = []


Is it simpler to enforce no white space allowed in the pattern?

White spaces are added automatically by the parser around parentheses:
https://github.com/cbouy/mdanalysis/blob/rdkit-converter/package/MDAnalysis/core/selection.py#L1182

jbarnoud · 2020-08-05T05:52:02Z

That is a super cool feature!

I did not read the code so maybe it is obvious and super well documented, but I have 2 questions:

what happens if I do not have RDKit installed?
what happens if the SMART string matches in several places? What is these places overlap?

cbouy · 2020-08-05T13:44:12Z

what happens if I do not have RDKit installed?

It raises an ImportError with a message that you need RDKit to use the SMARTS selection

what happens if the SMART string matches in several places? What if these places overlap?

You get a flat and "uniqued" AtomGroup with all atoms matching the query.

For example, with the molecule C(C=O)(C=C)C:

RDKit returns a tuple of matches for the query, i.e. if you query C-C it will return ((0, 1), (0, 3), (0, 5))
u.select_atoms("smarts C-C").indices will return array([0, 1, 3, 5])

There's a limit of 1000 matches (the default in RDKit).

Maybe it could be useful to add a kwarg to select_atoms so that it yields each match as a separate AtomGroup (i.e. [0, 1], then [0, 3]...etc.) ?

jbarnoud · 2020-08-05T13:47:38Z

Maybe it could be useful to add a kwarg to select_atoms so that it yields each match as a separate AtomGroup (i.e. [0, 1], then [0, 3]...etc.) ?

For API consistency's sake, it would be better to have an other method to do this. I think it would be very useful, but it is clearly for later.

cbouy · 2020-08-05T13:50:27Z

I agree that it could get confusing, a separate method is probably better

for ag in u.smarts_matches("C-C"):
    print(ag.indices)

richardjgowers

Code is fine, quick doc question

package/MDAnalysis/core/groups.py

…to smarts

IAlibay

Couple of things, but otherwise lgtm :)

package/MDAnalysis/core/groups.py

package/MDAnalysis/core/selection.py

testsuite/MDAnalysisTests/core/test_atomselections.py

fiona-naughton

Just one comment from me, otherwise looking good!

package/MDAnalysis/core/selection.py

IAlibay

Assuming CI returns green lgtm!

pinging @fiona-naughton and @richardjgowers for re-review, we can probably get this merged today/tomorrow.

codecov · 2020-08-26T17:55:41Z

Codecov Report

Merging #2883 into develop will increase coverage by 0.10%.
The diff coverage is 92.00%.

@@             Coverage Diff             @@
##           develop    #2883      +/-   ##
===========================================
+ Coverage    92.91%   93.01%   +0.10%     
===========================================
  Files          187      187              
  Lines        24591    24963     +372     
  Branches      3185     3263      +78     
===========================================
+ Hits         22848    23219     +371     
+ Misses        1697     1696       -1     
- Partials        46       48       +2

Impacted Files	Coverage Δ
package/MDAnalysis/core/groups.py	`98.58% <ø> (-0.01%)`	⬇️
package/MDAnalysis/core/selection.py	`99.24% <92.00%> (-0.26%)`	⬇️
package/MDAnalysis/coordinates/RDKit.py	`97.31% <0.00%> (-2.69%)`	⬇️
__init__.py	`91.89% <0.00%> (-0.61%)`	⬇️
package/MDAnalysis/__init__.py	`91.89% <0.00%> (-0.61%)`	⬇️
package/MDAnalysis/core/universe.py	`97.45% <0.00%> (+0.25%)`	⬆️
package/MDAnalysis/core/topologyattrs.py	`96.49% <0.00%> (+0.31%)`	⬆️
package/MDAnalysis/coordinates/base.py	`95.12% <0.00%> (+0.48%)`	⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 23074c6...1d2bb91. Read the comment docs.

Towards MDAnalysis#2468 ## Work done in this PR Adds a new SMARTS based selection which uses the RDKit converter.

add SMARTS selection

8ef63ac

pep8

ecd95e5

richardjgowers reviewed Jul 31, 2020

View reviewed changes

Cédric Bouysset added 2 commits July 31, 2020 19:54

add 'smarts' token to selections

517f9a5

revert convert_to (oops)

51ed059

richardjgowers reviewed Jul 31, 2020

View reviewed changes

Cédric Bouysset added 2 commits August 4, 2020 22:15

compatibility with grouping parentheses + documentation

b932512

more tests

b1596b3

orbeckst added the GSoC GSoC project label Aug 7, 2020

orbeckst assigned fiona-naughton and IAlibay Aug 7, 2020

Cédric Bouysset added 2 commits August 10, 2020 17:17

Merge branch 'develop' into smarts

350a995

fix smarts query from tokens

4c1bbfb

richardjgowers requested changes Aug 14, 2020

View reviewed changes

package/MDAnalysis/core/groups.py Show resolved Hide resolved

Cédric Bouysset added 2 commits August 18, 2020 20:11

Merge branch 'develop' of https://github.com/MDAnalysis/mdanalysis in…

8fcf9d2

…to smarts

add doc in .rst

5569155

IAlibay requested changes Aug 23, 2020

View reviewed changes

package/MDAnalysis/core/groups.py Show resolved Hide resolved

package/MDAnalysis/core/groups.py Outdated Show resolved Hide resolved

package/MDAnalysis/core/selection.py Outdated Show resolved Hide resolved

testsuite/MDAnalysisTests/core/test_atomselections.py Outdated Show resolved Hide resolved

fiona-naughton requested changes Aug 25, 2020

View reviewed changes

package/MDAnalysis/core/selection.py Outdated Show resolved Hide resolved

code review

1d2bb91

IAlibay approved these changes Aug 26, 2020

View reviewed changes

richardjgowers approved these changes Aug 28, 2020

View reviewed changes

fiona-naughton approved these changes Aug 28, 2020

View reviewed changes

IAlibay merged commit b2b7bcb into MDAnalysis:develop Aug 29, 2020

PicoCentauri pushed a commit to PicoCentauri/mdanalysis that referenced this pull request Mar 30, 2021

Add SMARTS selection (MDAnalysis#2883)

2bd4ea0

Towards MDAnalysis#2468 ## Work done in this PR Adds a new SMARTS based selection which uses the RDKit converter.

fiona-naughton added enhancement Component-Selections labels Sep 25, 2023

Conversation

cbouy commented Jul 31, 2020

PR Checklist

Uh oh!

pep8speaks commented Jul 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Comment last updated at 2020-08-26 16:53:09 UTC

Uh oh!

richardjgowers left a comment

Choose a reason for hiding this comment

Uh oh!

cbouy commented Jul 31, 2020

Uh oh!

cbouy commented Jul 31, 2020

Uh oh!

richardjgowers commented Jul 31, 2020

Uh oh!

richardjgowers Jul 31, 2020

Choose a reason for hiding this comment

Uh oh!

cbouy Aug 3, 2020

Choose a reason for hiding this comment

Uh oh!

richardjgowers Jul 31, 2020

Choose a reason for hiding this comment

Uh oh!

cbouy Aug 3, 2020

Choose a reason for hiding this comment

Uh oh!

richardjgowers Jul 31, 2020

Choose a reason for hiding this comment

Uh oh!

cbouy Aug 3, 2020

Choose a reason for hiding this comment

Uh oh!

jbarnoud commented Aug 5, 2020

Uh oh!

cbouy commented Aug 5, 2020

Uh oh!

jbarnoud commented Aug 5, 2020

Uh oh!

cbouy commented Aug 5, 2020

Uh oh!

richardjgowers left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

IAlibay left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fiona-naughton left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

IAlibay left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Aug 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

pep8speaks commented Jul 31, 2020 •

edited

Loading

codecov bot commented Aug 26, 2020 •

edited

Loading