Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
RDKit Descriptors and fingerprints wrapper #2912
base: develop
Are you sure you want to change the base?
RDKit Descriptors and fingerprints wrapper #2912
Changes from all commits
fc7ddcb
fd8529c
aa6e686
9f11ccc
5403e87
17cff1c
9fce129
7cadb00
844ff93
14f4302
09a4de0
41be3ad
9958770
37755d7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Maybe here we should link to the API docs for each of the fingerprints supported? We also should have a usage example for each case too (it adds to the docs but then users are clear on what should work).
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.
Or maybe just the one like to a page that lists all the individual docs for the fingerprints. Re: usage, if the usage is all similar/the same then we can just say that in the docs.
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.
As discussed, I personally think we should try to avoid these cases completely, but I'll let one of the other @MDAnalysis/gsoc-mentors make a judgement call here too.
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.
Ooo I really like this kind of on-the-fly Analysis construction. I think this probably the first time we're doing this in MDA, I like it, but other @MDAnalysis/coredevs might have opinions here.
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.
I like this example, however in the final version we will need something a little bit more descriptive, probably detailing what's in
_RDKIT_DESCRIPTORS
, any edge cases (are there any cases of descriptors that can be given optional parameters?), etc...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.
Like I said in
list_available
docstring, the list of descriptors is by no means curated, some of the things here aren't even descriptors but helper functions that are inside the module.Some of these functions take parameters like
includeHs
oronlyHeavy
, but most of them don't.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.
I'll defer to the other @MDAnalysis/gsoc-mentors here to pitch in on what they think might be a good compromise here.
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.
This needs documenting in the class docstring, so that users are aware of it's existence (including an example use).
I do wonder if it might be simpler (at the risk of making things harder for users), to not offer this list but rather just have the option of passing an RDKit function to this analysis method. Then you can just have this blank-ish AnalysisBase method that essentially is a pure trajectory analysis wrapper around existing RDKit function (if this makes any sense)? Again one of those things worth discussing further.
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.
Well it's definitely simpler for us but harder for users
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.
Porque no los dos, perhaps it could be a separate more flexible class and this can be the easy-peasy one.