Add BCUT 2D descriptors #2957
Add BCUT 2D descriptors #2957
Conversation
|
Pat Walters suggested I just try it on his ML test sets and if it is discriminatory, we call it a day. |
Code/GraphMol/Descriptors/BCUT.h
Outdated
| // copyright notice, this list of conditions and the following | ||
| // disclaimer in the documentation and/or other materials provided | ||
| // with the distribution. | ||
| // * Neither the name of Institue of Cancer Research. |
There was a problem hiding this comment.
Maybe you don't mean "ICR" here, right?
Maybe just put the usual boilerplate after the copyright?
// This file is part of the RDKit.
// The contents are covered by the terms of the BSD license
// which is included in the file license.txt, found at the root
// of the RDKit source tree.
|
@greglandrum It looks like this pulls in Eigen for the 2D descriptors which is failing a couple of builds. Any ideas? |
|
yeah, I had pointed out the new dependency as something we should think about in the review I started (but didn't finish) yesterday. |
|
@bp-kelley : when you're ready for the pre-merge review on this please just remove the |
| python::class_<std::pair<double, double> >("BCUTPair") | ||
| .def_readwrite("first", &std::pair<double, double>::first) | ||
| .def_readwrite("second", &std::pair<double, double>::second) | ||
| .def_readwrite("higest", &std::pair<double, double>::first) |
There was a problem hiding this comment.
I don't think including highest (which is mis-spelled here) and lowest add anything
| docString.c_str()); | ||
|
|
||
| python::class_<std::pair<double, double> >("BCUTPair") | ||
| .def_readwrite("first", &std::pair<double, double>::first) |
There was a problem hiding this comment.
why def_readwrite? does it make sense to change the values once they've been calculated in Python?
|
@greglandrum I think I'm done here |
| @@ -396,6 +400,54 @@ RDKit::SparseIntVect<std::uint32_t> *MorganFingerprintHelper( | |||
| } | |||
| return res; | |||
| } | |||
|
|
|||
| std::pair<double,double> BCUT2D_list(const RDKit::ROMol &m, python::list atomprops) | |||
There was a problem hiding this comment.
I believe this block also needs to be in an #ifdef RDK_HAS_EIGEN3 or it won't compile
|
@greglandrum the build failure doesn't look real. |
|
Yep; agreed. Please feel free to merge (it won’t let me merge on my phone of a test has failed) |
Preliminary BCUT descriptor implementation.
@greglandrum I'm not quite sure how to test this, there are a few different implementation styles and we won't have the same diagonal elements. Any ideas?
I'm leaning towards:
And maybe using a lookup table for polarizability.
One question is should we do absolute or carbon relative values?