Add a collection of new 3D descriptors #1467
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Initial refactoring.
* AutoCorr test added * RDF reviewed based on AutoCorr comments
* Morse reviewed
* correct MORSE test * start Getaway clean up
* better
* fix RCON
* fix the 3D autocorrelation descriptors based on the modification in Dragon. * Adding the 2D autocorrelation descriptors (no need of Eigen dependency for this one) * Adding 2D test case * IState … no idea for the moment * there is an error in 2D computation (memory error ???)
* need to use getTotalNumHs(true) not getTotalNumHs() * also need to remove Hs in both dv and d!
I fix the Rcov values
* remove push_back * remove call to sum * improve tests
* adding precision parameter to GETAWAY * adding rouding (1e-3)
- all modifications reduce computation time by a factor of 3!
fix the ATS and ATSC autocorrelation 2D
fix inversion in the computation of the equations for both M & G matrixes
fix the M & G matrixes computation
this is not correct all the cases
fix issue in cluster 0.01 0.009 case
…roduce the literature values exactly fix a leak in GETAWAY
this is strickly the implementation that we find in the book molecular descriptors for chemoinformatics (except the case where an atom is already in the axis in this case it should be added in the symetric list which is not the case in this implementation)
adding the axis atoms to the symetrical list
fix the negative values in gamma
|
Ton of work here. What is the plan for review? Perhaps we should do one valgrind run? |
|
Thanks for the reminder: I've run valgrind over all of the new regression tests and there are no warnings or leaks related to this code. |
greglandrum
changed the title
|
This is now in sync with master and I've added the new descriptors to the list in the Getting Started document. I think it's ready for merging. |
|
Many, many thanks to @thegodone for both the contribution and his patience during the very long review/revision process. |
greglandrum
added a commit
that referenced
this pull request
Jul 2, 2017
* 3D Descriptors Dragons * stripped down, not yet working * get this building on a non C++-11 compiler * move the python test to the python directory * move the python test to the python directory * add the python test * now at least those tests runn * warning comment * some basic refactoring and cleanup * get python wrapper "working" (completely untested) * fix Name * fixing AutoCorr & RDF * AutoCorr test added * RDF reviewed based on AutoCorr comments * fix Morse code * Morse reviewed * Correct Morse & start Getaway * correct MORSE test * start Getaway clean up * simplification of Whim * better * fix Getaway * fix RCON * merge repaired * adding Dragon 2D autocorrelations descriptors * fix the 3D autocorrelation descriptors based on the modification in Dragon. * Adding the 2D autocorrelation descriptors (no need of Eigen dependency for this one) * Adding 2D test case * IState … no idea for the moment * there is an error in 2D computation (memory error ???) * fix the IState for molecules with Hs * need to use getTotalNumHs(true) not getTotalNumHs() * also need to remove Hs in both dv and d! * fixing Rcov values I fix the Rcov values * fix Getaway * remove push_back * remove call to sum * improve tests * fix getaway * adding precision parameter to GETAWAY * adding rouding (1e-3) * fix WHIM * use void in declarations of function * update MolDescriptors link * remove print option in WHIM * fix python wrapper to 3D descriptors. - all modifications reduce computation time by a factor of 3! * final fix for Getaway * all output are fixed except the 2 first values due to clustering approach. * cluster cannot be fixed du tu float precision issue between Java & c++ * best fix of ITH and ISH * use the same algorithm as in Dragon 6 but there is still a deviation * remove std::move * std:move only works on c++ 11 * fixing issue based on Greg Comments * auto2D still not working on my env * update 3d test.py * auto 2D not working after the first loop test * tighten up the tests * change name * update, but still does not pass * make this run (though it does not work) * re-enable test3D * some cleanup * add GETAWAY test data. Note that the tests fail * fix the ATS and ATSC autocorrelation 2D Broto Moreau and Geary autocorrelation are not correct again a specificity of Dragon to compute them. The result are not consistant with Padel because we use the relative weigth not in Padel. * one minor change to get things to compile * fix the M & G matrix computation fix inversion in the computation of the equations for both M & G matrixes * update autocorr2d tests * 192 examples * fix issue in cluster 0.01 0.009 case this is not correct all the cases * update GETAWAY expected values to reflect the fact that we cannot reproduce the literature values exactly fix a leak in GETAWAY * fix the negative values in gamma this is strickly the implementation that we find in the book molecular descriptors for chemoinformatics (except the case where an atom is already in the axis in this case it should be added in the symetric list which is not the case in this implementation) * Update WHIM.cpp adding the axis atoms to the symetrical list * update WHIM tests * add AUTOCORR2D to MolDescriptors and the python wrappers * start adding tests * test the python versions of the new descriptors * update list of descriptors
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This should be viewed as a replacement for #1317
These should be viewed as beta still (and the docs need to be updated to reflect that before they are merged).
We will want to refactor the code as we have time, but we have regression tests here to make that doable.