opened
hroest
username_0
Warning: unknown N-terminal modification '2475.8875650638' - adding it to the database
Segmentation fault (core dumped)
```

it turns out that when parsing a sequence like N[2457.877]VSVK our `AASequence` "eats" the AA before the square brackets and when calling `toString` it outputs a sequence which is one AA shorter. Firstly, that is incorrect and secondly it means that repeated parsing of strings by OpenMS shorten it by 1 each time until we end up with a String like '[2475.8875650638]VSVK' which then leads to a segfault.

I looked at the code for quite some time and decided that it needed a major overhaul. This PR now substantially enhances our support for the bracket notation syntax which is also evident in the expanded AASequence test:

- `AASequence::fromString().toString()` not produces reproducibly the same result, no more "eating" of AA
- masses are calculated as expected
- `PEPT[+160.230654]IDE` gets correctly parsed as having a T with a modification of the correct mass (for residue as well as for full peptide)
- no more "funny" residues with arbitrary masses, now all modifications are true modifications with the underlying AA residue
- AASequence created from the same string compare as equal
- This allows `b = AASequence::fromString(a.toString())` such that `a == b`
- `.[1600.230654]IDE` gets parsed as N-terminal modification
- `IDE.[1600.230654]` gets parsed as C-terminal modification
Feature/fix square mods
issue
created
timosachsenberg
username_1
looks good. maybe a second opinion from @hendrikweisser or @enetz ? both have quite a good understanding of mod internals
comment
created
jpfeuffer
username_2
Can you have a look if it handles unknown AAs correctly now?
https://github.com/OpenMS/OpenMS/issues/2320
comment
created
jpfeuffer
username_2
Can you add e.g. in ResidueModification.h documentation on how you defined the new rules for using the (full)ID members? It seems like you use the rule that an unknown modification has no ID but a fullID with all the brackets and potentially Termspecifier? Currently there is no documentation on how to handle unknown ones, I think.

Regarding my last comment: Looking at the extensive tests, unknown AAs seem to be handled fine in toString.
Can you only additionally check toBracketString and toUnmodifiedString?
comment
created
jpfeuffer
username_2
Awesome! This looks great now.
comment
created
hroest
username_0
I also added the test from #2320 - there is only a single issue that I came across:

the mass of `DFPANGER` differs from `DFPANGERX` and `DFPANXGER` -> the latter are 18 Da *lighter* than the initial sequence. That is strange, it must be some weird interaction with the Internal/Full calculation of monoisotopic weight. I am not really sure what the expected behavior is but it is definitely unexpected. I will look into this some more ...
comment
created
hroest
username_0
maybe you can argue that you should never add an "X" amino acid without any mass -- but still...
comment
created
hroest
username_0
lets go ahead with this PR and put the discussion about `X` into #2532 where it belongs
comment