Just to note, the first one may not be a bug, but might be poor design. The second one is. That's why I was asking about issues (a Github issue would let me link to the commit)

Any fix? We had major problems before with tests used as feature requests... the number of failures quickly runs away.

Thats why I wanted to start an issue - the first one is possibly not a bug and maybe more of a design or documentation issue. I don't use this part of the code enough to know which way it should be, so while I can provide the fix, it may actually be the wrong thing to do

The second one appears to be a bug, but I don't have a fix right now.

The first one is a simple fix... but this is in the code...

// Verify if there is a previous calculation. If it exists, add the new
// isotopes

Want me to fix? It looks intentional.

I would argue the second one is a feature request. Unless the code is meant to handle charges it's not a bug. Given the progeny this code of is of I won't hold my breath on a fix and think the correct thing to do here is document the exception.

In fact... I don't even think the second one is fixable? The MF has no association of the element with the charge, and a charge does not always mean +1/-1 proton.

yes, regarding the first, I also saw that message. Based on that it looks intentional. In which case we should just update the document to indicate that if you cant reuse an instance of this class. If you do, then you need to make a new one.

Also is the test wrong...? Formulas work like this right... you don't add and remove H with a charge.

C1CCCCC1O.[Na] C6H12ONa
C1CCCCC1[O-].[Na] [C6H11ONa]⁻
CCCCCCO[Na] C6H13ONa
CCCCCC[O-].[Na] [C6H13ONa]⁻

The second one seems to be a bug. This is actually based on a report from rcdk (CDK-R/cdkr#38). Based on the example there, it looks like a bug. In the code itself, the function takes a IMolecularFormula object, but converts it to a string, ignoring the charge.

Hmm, you have a point.

Yeah me and Daniel were discussing this the other day in another context (which I can't remember) but basically there aren't any implicit hydrogens in formulas, the isotope pattern isn't adding/removing any.

And preempting a response on the rcdk... +1/-1 != proton +/-...

C[N+](C=CC=C1)=C1

Looking again that comment regarding the first one is because it does it one atom at a time so that can be updated.

yes, looks like the code doesn't handle charges explicitly, but does via formulae. So C6H11O6Na (negatively charged) does end up with a different mass than the neutral formula. So the code seems to be working (but should be documented for people like me :)

How would you document it...? Seems obvious to me because the charge isn't associated with an atom so we can't adjust/correct for it.

Indicate that setting the charge on the formula (since the formula object has a method to set charge) does not actually change anything. And instead, if you want a charged formula, specify the appropriate formula?

Okay done... however I also realised there is an elegant fix to the rcdk problem. Whilst setCharge should not change the hydrogen count... doesn't mean we can't have a convenience method to (de)protonate. I feel this can safely change both the charge and hydrogen count as it's all in the name and is clearly something a user would want to do.

Was tempted by mf.protonate(1) and mf.deprotonate(1) but the way CDK does isotopes is a bit clunky (i.e. created via factories) so I've made a method on the formula manipulator.

IChemObjectBuilder bldr = SilentChemObjectBuilder.getInstance();
IMolecularFormula mf = MolecularFormulaManipulator.getMolecularFormula("[C6H5O]-", bldr);
assertTrue(MolecularFormulaManipulator.adjustProtonation(mf, +1));
assertThat(MolecularFormulaManipulator.getString(mf), is("C6H6O"));
assertThat(mf.getCharge(), is(0));
assertThat(mf.getIsotopeCount(), is(3));

Ah, nice. This is what I had initially thought should be happening