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.

Sign up for GitHub

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

Issue #520: replace RedBlackMap for MoleculeAlleneStereo #818

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Issue #520: replace RedBlackMap for MoleculeAlleneStereo #818

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions core/indigo-core/molecule/molecule_allene_stereo.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@
#pragma warning(disable : 4251)
#endif

#include <functional>
#include <map>

#include "base_cpp/exception.h"
#include "base_cpp/red_black.h"
#include "math/algebra.h"
Expand All @@ -50,10 +53,7 @@ namespace indigo

bool isCenter(int atom_idx);
int size();
int begin() const;
int end() const;
int next(int i) const;
void get(int i, int& atom_idx, int& left, int& right, int subst[4], int& parity);
void forEach(std::function<void(int, int, int, const int[4], int)> callback) const;
void getByAtomIdx(int atom_idx, int& left, int& right, int subst[4], int& parity);
void invert(int atom_idx);
void reset(int atom_idx);
Expand Down Expand Up @@ -86,7 +86,7 @@ namespace indigo

bool _isAlleneCenter(BaseMolecule& mol, int idx, _Atom& atom, int* sensible_bonds_out);

RedBlackMap<int, _Atom> _centers;
std::map<int, _Atom> _centers;
Copy link
Member

Choose a reason for hiding this comment

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

That's what we discussed. We can't use standard containers as members of classes that are used inside Indigo memory-based containers. It leads to memory corruption. That's why the tests failed.

};

} // namespace indigo
Expand Down
108 changes: 46 additions & 62 deletions core/indigo-core/molecule/src/molecule_allene_stereo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,7 @@ void MoleculeAlleneStereo::buildFromBonds(BaseMolecule& baseMolecule, bool ignor
if (!ignore_errors)
throw err;
}
_centers.insert(i, atom);
_centers.emplace(i, atom);
}
}

Expand All @@ -293,7 +293,7 @@ void MoleculeAlleneStereo::clear()

bool MoleculeAlleneStereo::isCenter(int atom_idx)
{
return _centers.at2(atom_idx) != 0;
return _centers.find(atom_idx) != _centers.end();
}

void MoleculeAlleneStereo::invert(int atom_idx)
Expand All @@ -304,7 +304,7 @@ void MoleculeAlleneStereo::invert(int atom_idx)

void MoleculeAlleneStereo::reset(int atom_idx)
{
_centers.remove(atom_idx);
_centers.erase(atom_idx);
}

int MoleculeAlleneStereo::size()
Expand All @@ -318,21 +318,21 @@ bool MoleculeAlleneStereo::checkSub(BaseMolecule& query, BaseMolecule& target, c

for (i = query.vertexBegin(); i != query.vertexEnd(); i = query.vertexNext(i))
{
const _Atom* qa = query.allene_stereo._centers.at2(i);
const auto qa_it = query.allene_stereo._centers.find(i);

if (qa == 0)
if (qa_it == query.allene_stereo._centers.end())
continue;

const _Atom* ta = target.allene_stereo._centers.at2(mapping[i]);
const auto ta_it = target.allene_stereo._centers.find(mapping[i]);

if (ta == 0)
if (ta_it == query.allene_stereo._centers.end())
return false;

int parity = qa->parity;
int parity = qa_it->second.parity;
int qs[4], ts[4];

memcpy(qs, qa->subst, 4 * sizeof(int));
memcpy(ts, ta->subst, 4 * sizeof(int));
memcpy(qs, qa_it->second.subst, 4 * sizeof(int));
memcpy(ts, qa_it->second.subst, 4 * sizeof(int));

if (mapping[qs[0]] == ts[2] || mapping[qs[0]] == ts[3])
{
Expand All @@ -354,7 +354,7 @@ bool MoleculeAlleneStereo::checkSub(BaseMolecule& query, BaseMolecule& target, c
else
throw Error("checkSub() subst[2] not mapped");

if (parity != ta->parity)
if (parity != ta_it->second.parity)
return false;
}
return true;
Expand All @@ -364,10 +364,10 @@ void MoleculeAlleneStereo::buildOnSubmolecule(BaseMolecule& baseMolecule, BaseMo
{
int i, j;

for (i = super.allene_stereo._centers.begin(); i != super.allene_stereo._centers.end(); i = super.allene_stereo._centers.next(i))
for (const auto& pair : super.allene_stereo._centers)
{
int super_idx = super.allene_stereo._centers.key(i);
const _Atom& super_center = super.allene_stereo._centers.value(i);
int super_idx = pair.first;
const _Atom& super_center = pair.second;
int sub_idx = mapping[super_idx];

if (sub_idx < 0)
Expand Down Expand Up @@ -422,7 +422,7 @@ void MoleculeAlleneStereo::buildOnSubmolecule(BaseMolecule& baseMolecule, BaseMo
new_center.parity = 3 - new_center.parity;
}

_centers.insert(sub_idx, new_center);
_centers.emplace(sub_idx, new_center);

const Vertex& super_left = super.getVertex(super_center.left);
const Vertex& super_right = super.getVertex(super_center.right);
Expand Down Expand Up @@ -451,30 +451,14 @@ void MoleculeAlleneStereo::buildOnSubmolecule(BaseMolecule& baseMolecule, BaseMo
}
}

int MoleculeAlleneStereo::begin() const
void MoleculeAlleneStereo::forEach(std::function<void(int, int, int, const int[4], int)> callback) const
{
return _centers.begin();
}

int MoleculeAlleneStereo::end() const
{
return _centers.end();
}

int MoleculeAlleneStereo::next(int i) const
{
return _centers.next(i);
}

void MoleculeAlleneStereo::get(int i, int& atom_idx, int& left, int& right, int subst[4], int& parity)
{
_Atom& atom = _centers.value(i);

atom_idx = _centers.key(i);
left = atom.left;
right = atom.right;
parity = atom.parity;
memcpy(subst, atom.subst, sizeof(int) * 4);
for (const auto& pair : _centers)
{
int atom_idx = pair.first;
const _Atom& atom = pair.second;
callback(atom_idx, atom.left, atom.right, atom.subst, atom.parity);
}
}

void MoleculeAlleneStereo::getByAtomIdx(int atom_idx, int& left, int& right, int subst[4], int& parity)
Expand All @@ -496,16 +480,16 @@ void MoleculeAlleneStereo::add(int atom_idx, int left, int right, int subst[4],
memcpy(atom.subst, subst, 4 * sizeof(int));
atom.parity = parity;

_centers.insert(atom_idx, atom);
_centers.emplace(atom_idx, atom);
}

void MoleculeAlleneStereo::markBonds(BaseMolecule& baseMolecule)
{
int i, j;
for (i = _centers.begin(); i != _centers.end(); i = _centers.next(i))
for (const auto& pair : _centers)
{
int idx = _centers.key(i);
_Atom& atom = _centers.value(i);
int idx = pair.first;
const _Atom& atom = pair.second;

Vec3f subst_vecs[4];
int k;
Expand Down Expand Up @@ -658,27 +642,27 @@ void MoleculeAlleneStereo::removeAtoms(BaseMolecule& baseMolecule, const Array<i
{
int i, j;

QS_DEF(Array<int>, centers_to_remove);
QS_DEF(Array<decltype(_centers.begin())>, centers_to_remove);
centers_to_remove.clear();

for (i = 0; i < indices.size(); i++)
{
int idx = indices[i];
if (_centers.find(idx))
const auto it = _centers.find(idx);
if (it != _centers.end())
{
centers_to_remove.push(idx);
centers_to_remove.push(it);
continue;
}

// TODO: this can be done without looping through all centers
for (j = _centers.begin(); j != _centers.end(); j = _centers.next(j))
for (auto iter = _centers.begin(); iter != _centers.end(); ++iter)
{
int center_idx = _centers.key(j);
_Atom& atom = _centers.value(j);
_Atom& atom = iter->second;

if (idx == atom.left || idx == atom.right)
{
centers_to_remove.push(center_idx);
centers_to_remove.push(iter);
continue;
}

Expand All @@ -690,7 +674,7 @@ void MoleculeAlleneStereo::removeAtoms(BaseMolecule& baseMolecule, const Array<i
{
if (atom.subst[1] == -1 || (baseMolecule.getAtomNumber(atom.subst[1]) == ELEM_H && baseMolecule.possibleAtomIsotope(atom.subst[1], 0)))
{
centers_to_remove.push(center_idx);
centers_to_remove.push(iter);
continue;
}
atom.subst[0] = atom.subst[1];
Expand All @@ -700,7 +684,7 @@ void MoleculeAlleneStereo::removeAtoms(BaseMolecule& baseMolecule, const Array<i
{
if (atom.subst[3] == -1 || (baseMolecule.getAtomNumber(atom.subst[3]) == ELEM_H && baseMolecule.possibleAtomIsotope(atom.subst[3], 0)))
{
centers_to_remove.push(center_idx);
centers_to_remove.push(iter);
continue;
}
atom.subst[2] = atom.subst[3];
Expand All @@ -711,9 +695,7 @@ void MoleculeAlleneStereo::removeAtoms(BaseMolecule& baseMolecule, const Array<i

for (int i = 0; i < centers_to_remove.size(); i++)
{
int idx = centers_to_remove[i];
if (_centers.find(idx))
_centers.remove(idx);
_centers.erase(centers_to_remove[i]);
}
}

Expand All @@ -726,14 +708,14 @@ void MoleculeAlleneStereo::removeBonds(BaseMolecule& baseMolecule, const Array<i
int idx = indices[i];

// TODO: this can be done without looping through all centers
for (j = _centers.begin(); j != _centers.end(); j = _centers.next(j))
for (auto it = _centers.begin(); it != _centers.end();)
{
int center_idx = _centers.key(j);
_Atom& atom = _centers.value(j);
int center_idx = it->first;
_Atom& atom = it->second;

if (idx == baseMolecule.findEdgeIndex(center_idx, atom.left) || idx == baseMolecule.findEdgeIndex(center_idx, atom.right))
{
_centers.remove(center_idx);
it = _centers.erase(it);
continue;
}

Expand All @@ -745,7 +727,7 @@ void MoleculeAlleneStereo::removeBonds(BaseMolecule& baseMolecule, const Array<i
{
if (atom.subst[1] == -1 || (baseMolecule.getAtomNumber(atom.subst[1]) == ELEM_H && baseMolecule.possibleAtomIsotope(atom.subst[1], 0)))
{
_centers.remove(center_idx);
it = _centers.erase(it);
continue;
}
atom.subst[0] = atom.subst[1];
Expand All @@ -755,12 +737,14 @@ void MoleculeAlleneStereo::removeBonds(BaseMolecule& baseMolecule, const Array<i
{
if (atom.subst[3] == -1 || (baseMolecule.getAtomNumber(atom.subst[3]) == ELEM_H && baseMolecule.possibleAtomIsotope(atom.subst[3], 0)))
{
_centers.remove(center_idx);
it = _centers.erase(it);
continue;
}
atom.subst[2] = atom.subst[3];
atom.parity = 3 - atom.parity;
}

++it;
}
}
}
Expand All @@ -770,9 +754,9 @@ void MoleculeAlleneStereo::registerUnfoldedHydrogen(int atom_idx, int added_hydr
int j;

// TODO: this can be done without looping through all centers
for (j = _centers.begin(); j != _centers.end(); j = _centers.next(j))
for (auto pair : _centers)
{
_Atom& atom = _centers.value(j);
_Atom& atom = pair.second;

if (atom_idx == atom.left && atom.subst[1] == -1)
atom.subst[1] = added_hydrogen;
Expand Down
11 changes: 4 additions & 7 deletions core/indigo-core/molecule/src/smiles_saver.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,13 +328,10 @@ void SmilesSaver::_saveMolecule()

MoleculeAlleneStereo& allene_stereo = _bmol->allene_stereo;

for (i = allene_stereo.begin(); i != allene_stereo.end(); i = allene_stereo.next(i))
{
int atom_idx, left, right, parity, subst[4], subst_map[4] = {-1, -1, -1, -1};

allene_stereo.get(i, atom_idx, left, right, subst, parity);
allene_stereo.forEach([this](int atom_idx, int left, int right, const int subst[4], int parity) {
int subst_map[4] = {-1, -1, -1, -1};

for (j = 0; j < 4; j++)
for (int j = 0; j < 4; j++)
if (subst[j] >= 0)
subst_map[j] = _written_atoms_inv[subst[j]];

Expand Down Expand Up @@ -365,7 +362,7 @@ void SmilesSaver::_saveMolecule()
}

_atoms[atom_idx].chirality = 3 - parity;
}
});

if (canonize_chiralities)
{
Expand Down