Skip to content
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

Improve test coverage and some bug fixes #4536

Merged
merged 28 commits into from
Sep 26, 2021

Conversation

greglandrum
Copy link
Member

@greglandrum greglandrum commented Sep 19, 2021

This mainly adds tests to improve test coverage.

This is, of course, a story which could go on for a long time. Some improvement is better than none. :-)

Bug fixes made along the way:

  • copy-pasta in parseAdjustQueryParametersFromJSON()
  • copy-pasta in adjustQueryProperties()
  • Bond::getValenceContrib() would return non-zero results for atoms not involved in the bond
  • StereoType missing operator!=
  • DeprotectData missing operator!=
  • overly aggressive caching in getDistanceMat() and get3DDistanceMat()

This also deprecates the definition of BalabanJ which is in MolDiscriminators.cpp. If we want to keep this we should move it to the Descriptors directory.

@greglandrum greglandrum added bug enhancement Cleanup Code cleanup and refactoring labels Sep 19, 2021
@greglandrum greglandrum added this to the 2021_09_1 milestone Sep 19, 2021
(don't bother running some of the tests there)
Copy link
Contributor

@bp-kelley bp-kelley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two small questions, yeoman's work here greg, nice!

Code/GraphMol/Bond.cpp Show resolved Hide resolved
Code/GraphMol/Canon.cpp Show resolved Hide resolved
@@ -418,7 +413,13 @@ void test4() {
TEST_ASSERT(adjMat[1] == 1);
TEST_ASSERT(adjMat[2] == 1);
TEST_ASSERT(adjMat[3] == 0);
delete m;
bool useBO = true;
adjMat = MolOps::getAdjacencyMatrix(*m, useBO);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might want to check the prop names are being set properly here between useBO and atom Weights.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getAdjacencyMatrix() doesn't have an option with atom weights.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong function, I apparently meant: getDistanceMat

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that test does that already

@greglandrum greglandrum merged commit df72c24 into rdkit:master Sep 26, 2021
@greglandrum greglandrum deleted the dev/improve_coverage branch September 26, 2021 05:45
@ricrogz ricrogz mentioned this pull request Sep 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Cleanup Code cleanup and refactoring enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants