-
Notifications
You must be signed in to change notification settings - Fork 22
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
selection syntax #52
selection syntax #52
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #52 +/- ##
==========================================
+ Coverage 94.90% 95.08% +0.18%
==========================================
Files 9 10 +1
Lines 1786 1852 +66
==========================================
+ Hits 1695 1761 +66
Misses 91 91 ☔ View full report in Codecov by Sentry. |
(ps: now I see that it breaks 1.6 compatibility, but that is easily fixable). |
Looks great. We do have an API for selecting atoms, |
Added the One side question: Are you willing to use the If you are considering really to merge this, I can add more tests and write some documentation latter this week. |
I think this could be merged as it provides a nice interface. There is already some filtering code for Personally I'm not a fan of TestItems as I prefer the tests in one place. |
I think this is ready for review. I won't be available for the next few days. I removed the overload of What's missing from the selection syntax, relative to VMD:
In my opinion, if 3 and 4 are solved (probably someone that implemented a real parser of anything can do that in a morning), it would be very nice to move the parser to another package of more general utility, because the way the keywords and operators are implemented now are very modular and this could be used for other purposes. I commented the non-standard residue names that are used in MD simulations from the residue list. There is some redundancy relative to the selectors already available in BioStructures, but I'm not sure if sharing everything is worth the trouble or the reduced modularity. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, just a few comments.
There is some redundancy relative to the selectors already available in BioStructures, but I'm not sure if sharing everything is worth the trouble or the reduced modularity.
I might have a look after merging this about sharing more of the functions so that everything available here is available as a selector function and vice versa.
There's already Probably such information is or should be in a more fundamental package. |
Great, thanks for this. |
@jgreener64 @timholy
This PR is not necessarily to be merged (unless for the fact that it introduces something potentially useful and not breaking at all).
How this syntax works may be useful for the discussions of #48
I adapted the selection syntax as implemented in PDBTools for BioStructures.
With the PR, we can do (for example):
The syntax is still limited. We do not support parenthesis, and other features that VMD syntax supports. Ideally we would be able to parse something like
(resname ARG LYS) and (not backbone) and (chain A or model 2)
. Nevertheless, is what we have in PDBTools right now and that is useful for most of our purposes (and we switch to standard julia functions for more complicated things).This is related to the other discussion in the sense of what we want these selections to return. Now they return vectors of atoms, that is, flat structures. We could return (that would require some work) return a proper hierarchical structure, respecting the model, chain, residue, etc, of the original tree.
Food for thought. Hope this helps.