added test case in CDKAtomTypeMatcherFilesTest that gives rise to an NPE - #919

Bond Type 8 is a query bond (any) and not for representing molecules. We could support this with the ChemAxon extensions but this is not a bug but rather out of scope for now. It's the same with aromatic bonds (4).

Note V3000 has bond type 9 and 10 for dative and hbonds and this is much more portable.

Thank you for the explanation.

The file was read from V3000 and written to V2000 by ChemAxon.

The only thing I was looking for here is a more helpful exception. The NPE doesn't tell me anything about what is going on.

Would you consider it feasible to throw an UnsupportedOperationException instead just to make it clear what the problem is?

The only thing I was looking for here is a more helpful exception. The NPE doesn't tell me anything about what is going on.

This is what the Strict reading mode is about. I guess it should throw a CDKException with a readable message.

I can contribute to this one.

The file was read from V3000 and written to V2000 by ChemAxon.

Why not read the V3000 directly?

Why not read the V3000 directly?

Totally valid question 😃
That is probably me being very conservative and still assuming that V3000 isn't as widespread as V2000.
Seems like a good op to re-consider 🧠

Irrespective of that throwing a more informative exception might be worthwhile doing.

This is what the Strict reading mode is about. I guess it should throw a CDKException with a readable message.

I can contribute to this one.

@egonw Sounds good to me. Do you want me to move the test to MDLV2000ReaderTest?

If the idea is to throw a CDKException in case a query bond is encountered in Mode.STRICT what is the expected outcome when reading a query bond in Mode.RELAXED? Just log an error and move on?

Note it does give you back a query atom container...

    public static void main(String[] args) throws CDKException {
        String ctfile = "\n" +
                "  Mrv2204 10172221282D          \n" +
                "\n" +
                "  2  1  0  0  0  0            999 V2000\n" +
                "   -7.0097    0.3928    0.0000 N   0  0  0  0  0  0  0  0  0  0  0  0\n" +
                "   -6.1810    1.8211    0.0000 Ge  0  0  0  0  0  3  0  0  0  0  0  0\n" +
                "  1  2  8  0  0  0  0\n" +
                "M  STY  1   1 DAT\n" +
                "M  SAL   1  2   1   2\n" +
                "M  SDT   1 MRV_COORDINATE_BOND_TYPE                              \n" +
                "M  SDD   1     0.0000    0.0000    DR    ALL  0       0  \n" +
                "M  SED   1 1\n" +
                "M  END";
        MDLV2000Reader reader = new MDLV2000Reader(new StringReader(ctfile));
        IAtomContainer mol = reader.read(SilentChemObjectBuilder.getInstance().newAtomContainer());
        if (mol instanceof IQueryAtomContainer) {
            System.err.println("You have a query features in your molfile! I can't do much with it beyond using it as a query.");
        } else {
            // all ok!
        }
    }

It is sometimes possible to convert queries into molecules but we hesitate to do this automatically since it can introduce errors.

Note it does give you back a query atom container...

Uh, okay.
That doesn't make things easier, does it...?

Which brings me back to how this issue started: Does it make more sense to handle this in CDKAtomTypeMatcher? Maybe simply by ignoring atoms that are IQueryAtoms when atom types are perceived?

That doesn't make things easier, does it...?

If you try to treat a query as molecule you're going to have a bad time. It's simply the limit of what you can do in MOLfiles - short of customer ChemAxon extensions which are by no means defacto.

If you try to treat a query as molecule you're going to have a bad time. It's simply the limit of what you can do in MOLfiles - short of customer ChemAxon extensions which are by no means defacto.

Sorry for not making this clear: It is not my intention to treat query molecules as regular non-query molecules.

I definitely want to drop those query molecules within my processing pipeline. However, at the moment, they crash the flow of the pipeline by raising an exception and I was looking for a more specific (and thus also being a bit more informative) exception to catch instead of the NPE.

The obvious alternative seems to be that I more carefully validate what I am processing (i.e., by discarding any molecule that has query features). I am very happy to go down this route.

I definitely want to drop those query molecules within my processing pipeline. However, at the moment, they crash the flow of the pipeline by raising an exception and I was looking for a more specific (and thus also being a bit more informative) exception to catch instead of the NPE.

The obvious alternative seems to be that I more carefully validate what I am processing (i.e., by discarding any molecule that has query features). I am very happy to go down this route.

Yeah that's the intention, it gives you back a query molecule, if you have manual ways to clean that up then it may be possible to do something with it.

But yes running CdkAtomTypeMatcher on a query should throw a better error!

The test method CDKAtomTypeMatcherFilesTest::testGermaniumDativeBond does not crash anymore with an NPE.

I adjusted the expected types to {X, X} which seems fair given that they are connected by a dative bond.

@uli-f can you rebase this instead of merge?

@uli-f can you rebase this instead of merge?

That's what I was trying to do 🤷🏼

Not quite sure if it's IntelliJ or if that has sth to do with the branch sitting on a fork and then updating the fork, but I think it happened before with one of my PRs that was open for a while.

Sorry for the mess, do you have an idea what's best git-wise moving forward? Should I just close this one and open a new one with a patch of the actual changes....?

@johnmay Okay, got it sorted. And learned some git along the way, so that was a good exercise! Should be good to merge now.

Super thanks, yeah sorry I was going to try and figure out as well but it's more difficult when it's no on a repo I have write access too