MolComparator bug?

Target: [H]CCCC(C)=O

Query: [H]CC(=O)CCC

This code correctly returns false when exactExplicitHMatching is true:

    public static boolean matchPrecise(Molecule respMol, Molecule authMol,
            boolean exactExplicitHMatching, int stereoType)
            throws MolFileException {
        final String SELF = "MolFunctions.matchPrecise: ";
        debugPrint(SELF + "exactExplicitHMatching = ", exactExplicitHMatching,
                ", stereoType = ", stereoType);
        final MolSearch search = new MolSearch();
        final MolSearchOptions searchOpts = new MolSearchOptions();
        searchOpts.setSearchType(SearchConstants.DUPLICATE);
        searchOpts.setVagueBondLevel(SearchConstants.VAGUE_BOND_OFF);
        searchOpts.setStereoModel(SearchConstants.STEREO_MODEL_GLOBAL);
                // required for comparing nonaromatized aromatic rings
        final boolean ignore2D = ((stereoType & IGNORE_DBL_BOND_STEREO) != 0);
        final boolean ignore3D = ((stereoType & IGNORE_TETRAHEDRAL_STEREO) != 0);
        final boolean wavyAnd = ((stereoType & WAVY_AND) != 0);
        boolean addWavyBondMatcher = false;
        if (ignore2D) {
            debugPrint(SELF + "ignoring 2D stereochemistry.");
            searchOpts.setDoubleBondStereoMatchingMode(StereoConstants.DBS_NONE);
        } // if ignore2D
        if (ignore3D) {
            debugPrint(SELF + "ignoring 3D stereochemistry.");
            searchOpts.setStereoSearchType(SearchConstants.STEREO_IGNORE);
        } else if (wavyAnd) {
            addWavyBondMatcher = true;
            // following line needed to avoid bug in JChem 5.1.3_2 and earlier
            searchOpts.setKeepQueryOrder(true);
        } // if ignore3D
        search.setSearchOptions(searchOpts);
        if (addWavyBondMatcher) {
            debugPrint(SELF + "adding WavyBondMatcher.");
            search.addComparator(new WavyBondMatcher());
        } else debugPrint(SELF + "not adding WavyBondMatcher.");
        if (exactExplicitHMatching) {
            debugPrint(SELF + "adding ExplicitHMatcher.");
            search.addComparator(new ExplicitHMatcher());
        } else debugPrint(SELF + "not adding ExplicitHMatcher.");
        search.setTarget(respMol);
        search.setQuery(authMol);
        debugPrintMRV(SELF + "respMol:\n", respMol);
        debugPrintMRV(SELF + "authMol:\n", authMol);
        try {
            final boolean match = search.isMatching();
            debugPrint(SELF + "search result is ", match);
            return match;
        } catch (SearchException e2) {
            Utils.alwaysPrint("Error in " + SELF);
            e2.printStackTrace();
            throw new MolFileException(ERROR + e2.getMessage());
        }
    } // matchPrecise(Molecule, Molecule, boolean)

This code incorrectly returns true when exactExplicitHMatching is true:

    public static boolean matchPrecise(Molecule respMol, Molecule authMol,

            boolean exactExplicitHMatching, int stereoType)

            throws MolFileException {

        final String SELF = "MolFunctions.matchPrecise: ";

        debugPrint(SELF + "exactExplicitHMatching = ", exactExplicitHMatching,

                ", stereoType = ", stereoType);

        final MolSearch search = new MolSearch();

        final MolSearchOptions searchOpts = new MolSearchOptions();

        searchOpts.setSearchType(SearchConstants.DUPLICATE);

        searchOpts.setVagueBondLevel(SearchConstants.VAGUE_BOND_OFF);

        searchOpts.setStereoModel(SearchConstants.STEREO_MODEL_GLOBAL);

                // required for comparing nonaromatized aromatic rings

        final boolean ignore2D = ((stereoType & IGNORE_DBL_BOND_STEREO) != 0);

        final boolean ignore3D = ((stereoType & IGNORE_TETRAHEDRAL_STEREO) != 0);

        final boolean wavyAnd = ((stereoType & WAVY_AND) != 0);

        boolean addWavyBondMatcher = false;

        if (exactExplicitHMatching) {

            debugPrint(SELF + "adding ExplicitHMatcher.");

            search.addComparator(new ExplicitHMatcher());

        } else debugPrint(SELF + "not adding ExplicitHMatcher.");

        if (ignore2D) {

            debugPrint(SELF + "ignoring 2D stereochemistry.");

            searchOpts.setDoubleBondStereoMatchingMode(StereoConstants.DBS_NONE);

        } // if ignore2D

        if (ignore3D) {

            debugPrint(SELF + "ignoring 3D stereochemistry.");

            searchOpts.setStereoSearchType(SearchConstants.STEREO_IGNORE);

        } else if (wavyAnd) {

            addWavyBondMatcher = true;

            // following line needed to avoid bug in JChem 5.1.3_2 and earlier

            searchOpts.setKeepQueryOrder(true);

        } // if ignore3D

        search.setSearchOptions(searchOpts);

        if (addWavyBondMatcher) {

            debugPrint(SELF + "adding WavyBondMatcher.");

            search.addComparator(new WavyBondMatcher());

        } else debugPrint(SELF + "not adding WavyBondMatcher.");

        search.setTarget(respMol);

        search.setQuery(authMol);

        debugPrintMRV(SELF + "respMol:\n", respMol);

        debugPrintMRV(SELF + "authMol:\n", authMol);

        try {

            final boolean match = search.isMatching();

            debugPrint(SELF + "search result is ", match);

            return match;

        } catch (SearchException e2) {

            Utils.alwaysPrint("Error in " + SELF);

            e2.printStackTrace();

            throw new MolFileException(ERROR + e2.getMessage());

        }

    } // matchPrecise(Molecule, Molecule, boolean)

The only difference is whether the ExplicitHMatcher comparator is added to the MolSearch object before or after some of the MolSearchOptions are added.  The comparator is disabled (debug output shows it is never called) if the search options are added to the MolSearch object after the comparator is added to it.  

I have trouble believing this is anything but a bug.

Code of ExplicitHMatcher.compareAtoms():

    public boolean compareAtoms(int queryAtomNum, int targetAtomNum) {

        debugPrint("Entering ExplicitHMatcher.compareAtoms: "

                + "query = ", query, ", target = ", target);

        final int qAtomNumOrig = getOrigQueryAtom(queryAtomNum);

        int qHCount = 0;

        if (qAtomNumOrig != -1) {

            final MolAtom qAtom = query.getAtom(qAtomNumOrig);

            qHCount = qAtom.getExplicitHcount();

            debugPrint("qAtom ", qAtom, qAtomNumOrig + 1,

                    " has ", qHCount, " explicit H atoms.");

        } else debugPrint("qAtom H is implicit.");

        final int tAtomNumOrig = getOrigTargetAtom(targetAtomNum);

        int tHCount = 0;

        if (tAtomNumOrig != -1) {

            final MolAtom tAtom = target.getAtom(tAtomNumOrig);

            tHCount = tAtom.getExplicitHcount();

            debugPrint("tAtom ", tAtom, tAtomNumOrig + 1,

                    " has ", tHCount, " explicit H atoms.");

        } else debugPrint("tAtom H is implicit.");

        final boolean match = (qHCount == tHCount);

        debugPrint(match ? "Atoms match." : "Atoms do not match.");

        return match;

    } // compareAtoms(int, int)

Hi Bob,

MolComparators are stored (referenced) in MolSearch's SearchOption object.

When you first add a custom MolComparator, the later MolSearch.setSearchOptions call overwrites the stored MolComparators (with null in this case).

When you first call MolSearch.setSearchOptions and then add the custom MolComparator it works fine.

The preferred way to add MolComparators would be first calling SearchOptions.addUserComparator and MolSearch.setSearchOptions only after it. As there is a slight design error in MolSearch - superflouos and confusing addComparator() method - , we will also deprecate this method in next release.

Regards,
Tamás

Ah, I see.  I did not think that changing anything in SearchOptions would affect the contents of MolSearch that were set independently of SearchOptions.  But apparently, that is not the case.

I recommend that not only do you deprecate MolSearch.addComparator(), but you document in the API that SearchOptions.addUserComparator() should be called after other SearchOptions are set but before the SearchOptions are added to the MolSearch.  I think it would also be very useful to have some documentation of how setting some options can unexpectedly change the values of other options.  

I also recommend you make new Search and MolSearch constructors, Search(SearchOptions) and MolSearch(SearchOptions).  It would save a line of code and make the code a little more readable.

bobgr wrote:

... SearchOptions.addUserComparator() should be called after other SearchOptions are set but before the SearchOptions are added to the MolSearch.  I think it would also be very useful to have some documentation of how setting some options can unexpectedly change the values of other options.   ...

Hi Bob,

SearchOptions.addUserComparator() can be used before or after setting other search options, it won't change any other settings. The important is that you first set options (search options and/or comparators) in SearchOptions/MolSearchOptions and call MolSearch.setSearchOptions() only when you are ready with all settings.

We will consider your suggestions about introducing new constructors.

Regards,
Tamás