pub_soft/rdkit
3
0
Fork 0
mirror of https://github.com/rdkit/rdkit.git synced 2026-06-03 21:44:30 +08:00
Code Issues Packages Projects Releases Wiki Activity

Mem checkup & fixes (#3510)

Browse Source 
* fix leak in testConformerParser

* fix leaks in testMultithreadedMolSupplier

* fix leak in catch_graphmol

* pass build type to YAEHMOP

* cleanup fragments in CoordGen minimizeOnly

* fix leaking ConjElectrons stack in res mol supplier

* avoid double delete

* do not delete 'this'; clean ce not added to map

* delete mol if Multithreaded SD readMolProps throws

* fix typo

* fix typo in comment
This commit is contained in:
Ric
2020-11-09 12:08:36 -05:00
committed by GitHub
parent b2c3cd935a
commit 2afb4fbac4
7 changed files with 62 additions and 44 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
Code/GraphMol/FileParsers/MultithreadedSDMolSupplier.cpp
View File

@@ -183,6 +183,9 @@ void MultithreadedSDMolSupplier::readMolProps(ROMol *mol,
while (stmp.length() != 0) {
std::getline(inStream, tempStr);
if (inStream.eof()) {
if (mol) {
delete mol;
}
throw FileParseException("End of data field name not found");
}
}
@@ -228,6 +231,9 @@ void MultithreadedSDMolSupplier::readMolProps(ROMol *mol,
// and issue a warning
// FIX: should we be deleting the molecule (which is probably fine)
// because we couldn't read the data ???
if (mol) {
delete mol;
}
throw FileParseException("Problems encountered parsing data fields");
} else {
if (!warningIssued) {

37
Code/GraphMol/FileParsers/testMultithreadedMolSupplier.cpp
View File

@@ -7,6 +7,10 @@
// which is included in the file license.txt, found at the root
// of the RDKit source tree.
//
#include <chrono>
#include <memory>
#include <GraphMol/Fingerprints/MorganFingerprints.h>
#include <GraphMol/SmilesParse/SmilesWrite.h>
#include <RDGeneral/test.h>
@@ -15,7 +19,6 @@
#include <boost/dynamic_bitset.hpp>
#include <boost/iostreams/device/file.hpp>
#include <boost/iostreams/filtering_stream.hpp>
#include <chrono>
#include "MultithreadedSDMolSupplier.h"
#include "MultithreadedSmilesMolSupplier.h"
@@ -34,7 +37,7 @@ struct PrintThread : public std::stringstream {
}
};
void testSmiConcurrent(std::istream* strm, bool takeOwnership,
void testSmiConcurrent(std::istream *strm, bool takeOwnership,
std::string delimiter, int smilesColumn, int nameColumn,
bool titleLine, bool sanitize,
unsigned int numWriterThreads, size_t sizeInputQueue,
@@ -51,7 +54,7 @@ void testSmiConcurrent(std::istream* strm, bool takeOwnership,
TEST_ASSERT(!bitVector.any());
while (!sup.atEnd()) {
ROMol* mol = sup.next();
ROMol *mol = sup.next();
if (mol) {
unsigned int id = sup.getLastRecordId();
bitVector[id - 1] = 1;
@@ -75,7 +78,7 @@ void testSmiConcurrent(std::string path, std::string delimiter,
unsigned int expectedResult, bool extras = false) {
std::string rdbase = getenv("RDBASE");
std::string fname = rdbase + path;
std::istream* strm = new std::ifstream(fname.c_str());
std::istream *strm = new std::ifstream(fname.c_str());
testSmiConcurrent(strm, true, delimiter, smilesColumn, nameColumn, titleLine,
sanitize, numWriterThreads, sizeInputQueue, sizeOutputQueue,
expectedResult, extras);
@@ -88,7 +91,7 @@ void testSmiOld(std::string path, std::string delimiter, int smilesColumn,
SmilesMolSupplier sup(path, delimiter, smilesColumn, nameColumn, titleLine,
sanitize);
while (!sup.atEnd()) {
ROMol* mol = sup.next();
ROMol *mol = sup.next();
if (mol) {
if (extras) {
std::unique_ptr<ExplicitBitVect> fp(
@@ -110,7 +113,7 @@ void testSmiProperties() {
SmilesMolSupplier sup(fname, ",", 1, 0, true);
MultithreadedSmilesMolSupplier multiSup(fname, ",", 1, 0, true);
while (!multiSup.atEnd()) {
ROMol* mol = multiSup.next();
std::unique_ptr<ROMol> mol{multiSup.next()};
if (mol != nullptr) {
mol->getProp(common_properties::_Name, tempStr);
nameVector.push_back(tempStr);
@@ -120,7 +123,7 @@ void testSmiProperties() {
}
while (!sup.atEnd()) {
ROMol* mol = sup.next();
std::unique_ptr<ROMol> mol{sup.next()};
if (mol != nullptr) {
mol->getProp(common_properties::_Name, tempStr);
TEST_ASSERT(std::find(nameVector.begin(), nameVector.end(), tempStr) !=
@@ -153,7 +156,7 @@ void testSmiCorrectness() {
#ifdef RDK_USE_BOOST_IOSTREAMS
path = rdbase + "/Regress/Data/znp.50k.smi.gz";
std::istream* strm = new gzstream(path);
std::istream *strm = new gzstream(path);
expectedResult = 50000;
testSmiConcurrent(strm, true, " \t", 0, 1, false, false, 3, 1000, 100,
expectedResult);
@@ -166,7 +169,7 @@ void testSmiCorrectness() {
testSmiProperties();
}
void testSDConcurrent(std::istream* strm, bool takeOwnership, bool sanitize,
void testSDConcurrent(std::istream *strm, bool takeOwnership, bool sanitize,
bool removeHs, bool strictParsing,
unsigned int numWriterThreads, size_t sizeInputQueue,
size_t sizeOutputQueue, unsigned int expectedResult,
@@ -181,7 +184,7 @@ void testSDConcurrent(std::istream* strm, bool takeOwnership, bool sanitize,
// initially no bit is set in the bitVector, sanity check
TEST_ASSERT(!bitVector.any());
while (!sup.atEnd()) {
ROMol* mol = sup.next();
ROMol *mol = sup.next();
if (mol) {
unsigned int id = sup.getLastRecordId();
bitVector[id - 1] = 1;
@@ -204,7 +207,7 @@ void testSDConcurrent(std::string path, bool sanitize, bool removeHs,
unsigned int expectedResult, bool extras = false) {
std::string rdbase = getenv("RDBASE");
std::string fname = rdbase + path;
std::istream* strm = new std::ifstream(fname.c_str());
std::istream *strm = new std::ifstream(fname.c_str());
testSDConcurrent(strm, true, sanitize, removeHs, strictParsing,
numWriterThreads, sizeInputQueue, sizeOutputQueue,
expectedResult, extras);
@@ -220,7 +223,7 @@ void testSDProperties() {
MultithreadedSDMolSupplier multiSup(fname, false);
while (!multiSup.atEnd()) {
ROMol* mol = multiSup.next();
std::unique_ptr<ROMol> mol{multiSup.next()};
if (mol != nullptr) {
TEST_ASSERT(mol->hasProp(common_properties::_Name));
mol->getProp(common_properties::_Name, tempStr);
@@ -232,7 +235,7 @@ void testSDProperties() {
}
while (!sup.atEnd()) {
ROMol* mol = sup.next();
std::unique_ptr<ROMol> mol{sup.next()};
if (mol != nullptr) {
mol->getProp(common_properties::_Name, tempStr);
TEST_ASSERT(std::find(nameVector.begin(), nameVector.end(), tempStr) !=
@@ -247,7 +250,7 @@ void testSDOld(std::string path, bool sanitize, bool removeHs,
unsigned int numMols = 0;
SDMolSupplier sup(path, sanitize, removeHs, strictParsing);
while (!sup.atEnd()) {
ROMol* mol = sup.next();
ROMol *mol = sup.next();
if (mol) {
++numMols;
if (extras) {
@@ -290,7 +293,7 @@ void testSDCorrectness() {
#ifdef RDK_USE_BOOST_IOSTREAMS
path = rdbase + "/Regress/Data/mols.1000.sdf.gz";
std::istream* strm = new gzstream(path);
std::istream *strm = new gzstream(path);
expectedResult = 1000;
testSDConcurrent(strm, true, false, true, true, 2, 5, 5, expectedResult);
@@ -337,7 +340,7 @@ void testPerformance() {
<< " (milliseconds) \n";
for (unsigned int i = maxThreadCount; i >= 1; --i) {
std::istream* strm = new gzstream(rdbase + gzpath);
std::istream *strm = new gzstream(rdbase + gzpath);
start = high_resolution_clock::now();
bool takeOwnership = true;
testSmiConcurrent(strm, takeOwnership, delim, smilesColumn, nameColumn,
@@ -374,7 +377,7 @@ void testPerformance() {
<< " (milliseconds) \n";
for (unsigned int i = maxThreadCount; i >= 1; --i) {
std::istream* strm = new gzstream(rdbase + gzpath);
std::istream *strm = new gzstream(rdbase + gzpath);
bool takeOwnership = true;
start = high_resolution_clock::now();
testSDConcurrent(strm, takeOwnership, sanitize, removeHs, strictParsing,

52
Code/GraphMol/Resonance.cpp
View File

@@ -132,7 +132,7 @@ class ConjElectrons {
bool checkChargesAndBondOrders();
void computeMetrics();
bool checkMetrics(CEStats &ceStats, bool &ok) const;
void purgeMaps(CEMap &ceMap, CEDegCount &ceDegCount, CEStats &ceStats) const;
bool purgeMaps(CEMap &ceMap, CEDegCount &ceDegCount, CEStats &ceStats) const;
void updateDegCount(CEDegCount &ceDegCount);
std::size_t computeFP(unsigned int flags);
inline bool haveFP(CESet &ceSet, unsigned int flags);
@@ -296,11 +296,7 @@ void fixExplicitImplicitHs(ROMol &mol) {
// object constructor
AtomElectrons::AtomElectrons(ConjElectrons *parent, const Atom *a)
: d_nb(0),
d_fc(0),
d_flags(0),
d_atom(a),
d_parent(parent) {
: d_nb(0), d_fc(0), d_flags(0), d_atom(a), d_parent(parent) {
PRECONDITION(d_atom, "d_atom cannot be NULL");
d_tv = static_cast<std::uint8_t>(a->getTotalDegree());
const ROMol &mol = d_atom->getOwningMol();
@@ -833,23 +829,28 @@ bool ConjElectrons::checkMetrics(CEStats &ceStats, bool &changed) const {
return ok;
}
void ConjElectrons::purgeMaps(CEMap &ceMap, CEDegCount &ceDegCount,
bool ConjElectrons::purgeMaps(CEMap &ceMap, CEDegCount &ceDegCount,
CEStats &ceStats) const {
bool ok = true;
bool changed = true;
while (changed) {
for (CEMap::iterator it = ceMap.begin(); it != ceMap.end();) {
for (auto it = ceMap.begin(); it != ceMap.end();) {
if (!it->second->checkMetrics(ceStats, changed)) {
CEMap::iterator toBeDeleted = it;
++it;
auto it2 = ceDegCount.find(toBeDeleted->second->hash());
auto it2 = ceDegCount.find(it->second->hash());
if (it2 != ceDegCount.end()) {
--it2->second;
if (!it2->second) {
ceDegCount.erase(it2);
}
}
delete toBeDeleted->second;
ceMap.erase(toBeDeleted);
if (it->second == this) {
// postpone self deletion
ok = false;
} else {
delete it->second;
}
it = ceMap.erase(it);
changed = true;
} else {
++it;
}
@@ -858,6 +859,7 @@ void ConjElectrons::purgeMaps(CEMap &ceMap, CEDegCount &ceDegCount,
}
}
}
return ok;
}
// assign formal charges and, if they are acceptable, store
@@ -877,7 +879,7 @@ bool ConjElectrons::assignFormalChargesAndStore(CEMap &ceMap,
ok = storeFP(ceMap, fpFlags);
}
if (changed) {
purgeMaps(ceMap, ceDegCount, ceStats);
ok = purgeMaps(ceMap, ceDegCount, ceStats) && ok;
}
if (ok) {
updateDegCount(ceDegCount);
@@ -888,7 +890,6 @@ bool ConjElectrons::assignFormalChargesAndStore(CEMap &ceMap,
// enumerate all possible permutations of non-bonded electrons
void ConjElectrons::enumerateNonBonded(CEMap &ceMap, CEDegCount &ceDegCount,
CEStats &ceStats) {
ConjElectrons *ce = this;
// the way we compute FPs for a resonance structure depends
// on whether we want to enumerate all Kekule structures
// or not; in the first case, we also include bond orders
@@ -926,12 +927,10 @@ void ConjElectrons::enumerateNonBonded(CEMap &ceMap, CEDegCount &ceDegCount,
ResonanceUtils::getNumCombStartV(numCand, aiVec.size(), numComb, v);
// if there are multiple permutations, make a copy of the original
// ConjElectrons object, since the latter will be modified
ConjElectrons *ceCopy = ((numComb > 1) ? new ConjElectrons(*ce) : nullptr);
ConjElectrons *ceCopy = new ConjElectrons(*this);
// enumerate all permutations
for (unsigned int c = 0; c < numComb; ++c) {
if (c) {
ce = new ConjElectrons(*ceCopy);
}
ConjElectrons *ce = new ConjElectrons(*ceCopy);
unsigned int vc = v;
for (unsigned int i : aiVec) {
AtomElectrons *ae = ce->getAtomElectronsWithIdx(i);
@@ -951,11 +950,13 @@ void ConjElectrons::enumerateNonBonded(CEMap &ceMap, CEDegCount &ceDegCount,
fpFlags)) {
delete ce;
}
// get the next binary code
ResonanceUtils::updateV(v);
}
delete ceCopy;
} else if (nbTotal == currElectrons()) {
ConjElectrons *ce = new ConjElectrons(*this);
// if the electrons required to satisfy all octets
// are as many as those currently available, assignment
// is univocal
@@ -964,12 +965,10 @@ void ConjElectrons::enumerateNonBonded(CEMap &ceMap, CEDegCount &ceDegCount,
if (!ce->assignFormalChargesAndStore(ceMap, ceDegCount, ceStats, fpFlags)) {
delete ce;
}
} else {
// if the electrons required to satisfy all octets are less
// than those currently available, we must have failed the bond
// assignment, so the candidate must be deleted
delete ce;
}
// if the electrons required to satisfy all octets are less
// than those currently available, we must have failed the bond
// assignment
}
void ConjElectrons::computeMetrics() {
@@ -1694,6 +1693,7 @@ void ResonanceMolSupplier::buildCEMap(CEMap &ceMap, unsigned int conjGrpIdx) {
// enumerate possible non-bonded electron
// arrangements, and store them in ceMap
ce->enumerateNonBonded(ceMap, ceDegCount, ceStats);
delete ce;
// quit the loop early if the number of non-degenerate
// structures already exceeds d_maxStructs
if (d_callback.get()) {
@@ -1711,6 +1711,10 @@ void ResonanceMolSupplier::buildCEMap(CEMap &ceMap, unsigned int conjGrpIdx) {
}
}
}
while (!ceStack.empty()) {
delete ceStack.top();
ceStack.pop();
}
}
// getter function which returns the bondConjGrpIdx for a given

2
Code/GraphMol/catch_graphmol.cpp
View File

@@ -1380,7 +1380,7 @@ TEST_CASE("Github #3470: Hydrogen is incorrectly identified as an early atom",
"[bug][chemistry]") {
SECTION("Basics") {
RWMol m;
m.addAtom(new Atom(1));
m.addAtom(new Atom(1), true, true);
m.getAtomWithIdx(0)->setFormalCharge(-1);
m.updatePropertyCache();
CHECK(m.getAtomWithIdx(0)->getNumImplicitHs() == 0);

3
Contrib/ConformerParser/test.cpp
View File

@@ -33,6 +33,7 @@
#include <iostream>
#include <fstream>
#include <memory>
#include <RDGeneral/Invariant.h>
#include <RDGeneral/RDLog.h>
@@ -57,7 +58,7 @@ void test1() {
BOOST_LOG(rdErrorLog) << "-------------------------------------" << std::endl;
BOOST_LOG(rdErrorLog) << " Test ConformerParser." << std::endl;
ROMol *mol = SmilesToMol("CCC");
std::unique_ptr<ROMol> mol{SmilesToMol("CCC")};
std::vector<std::vector<double>> coords;
std::string rdbase = getenv("RDBASE");
std::string fName = rdbase + "/Code/GraphMol/test_data/water_coords_bad.trx";

4
External/CoordGen/CoordGen.h vendored
View File

@@ -215,6 +215,10 @@ unsigned int addCoords(T& mol, const CoordGenParams* params = nullptr) {
} else {
CoordgenFragmenter::splitIntoFragments(min_mol);
minimizer.m_minimizer.minimizeMolecule(min_mol);
// Hook the fragments into the minimizer so that they
// get cleaned up when the minimizer is terminated
minimizer._fragments = min_mol->getFragments();
}
auto conf = new Conformer(mol.getNumAtoms());
for (size_t i = 0; i < mol.getNumAtoms(); ++i) {

2
External/YAeHMOP/CMakeLists.txt vendored
View File

@@ -24,7 +24,7 @@ ExternalProject_Add(yaehmop_project
PREFIX ${CMAKE_CURRENT_SOURCE_DIR}
SOURCE_DIR "${CMAKE_CURRENT_SOURCE_DIR}/yaehmop"
SOURCE_SUBDIR "tightbind"
CMAKE_ARGS -DUSE_BLAS_LAPACK=OFF -DCMAKE_INSTALL_PREFIX=${PROJECT_BINARY_DIR} -DCMAKE_C_FLAGS=${CMAKE_C_FLAGS}
CMAKE_ARGS -DUSE_BLAS_LAPACK=OFF -DCMAKE_INSTALL_PREFIX=${PROJECT_BINARY_DIR} -DCMAKE_C_FLAGS=${CMAKE_C_FLAGS} -DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}
TEST_COMMAND "")
include_directories(${PROJECT_BINARY_DIR}/include)