Skip to content

Include positional modifications in calculate fragments#37

Open
guideflandre wants to merge 24 commits intorformassspectrometry:mainfrom
guideflandre:calculateFragments
Open

Include positional modifications in calculate fragments#37
guideflandre wants to merge 24 commits intorformassspectrometry:mainfrom
guideflandre:calculateFragments

Conversation

@guideflandre
Copy link
Contributor

@guideflandre guideflandre commented Dec 18, 2025

This PR is linked to #17 and is meant to support positional modifications in calculateFragments.

I have encountered multiple situations for which this was needed and it also resolves some of the issues that were posted regarding positional modifications #14 and #15 .

Much like #17 , the function only accepts delta mass annotation styles (i.e. "PEPT[+79.966]IDEK]").

The function still allows the use of fixed modifications (i.e. fixed_modifications = c(C = 57.02146) by default) which can cause wrong fragment masses if fixed modifications are explicitly written in the given sequences.
(i.e. "THEFASTC[+57.02146]AT" would add carbamidomethylation twice if fixed_modifications is not explicitly set to NULL.
I could add a warning when both positional modifications and fixed modifications are used to remedy this.

The function throws an error if both positional and variable modifications are used.

PS:
In addition to these positional modifications, I have created a ProForma parser limited to unimod ids, unimod names and delta mass annotations (allowing to convert from one annotation style to any of the others). The function is based on the modifications dataframe from the unimod package and the changes are located in this repo from the unimod package. The parser is very useful to me personally, and is very easy to integrate within calculateFragments.

HOWEVER, the unimod package is not part of bioconductor and must be installed from github. This makes it not user-friendly.

This PR is basically a continuation of the discussion in #17 : the integration of a ProForma parser (although limited to unimod ids, names and delta masses) makes my life easier and is easily integrated with calculateFragments, but not with bioconductor.

I have created an issue in the unimod package repo to discuss this matter there in more details.

PSS:
I adjusted the masses for selenocysteine and pyrrolysine from getAminoAcids() based on this table.

@lgatto lgatto requested review from lgatto and sgibb December 20, 2025 12:45
@lgatto lgatto marked this pull request as draft December 20, 2025 12:49
@lgatto
Copy link
Member

lgatto commented Dec 20, 2025

@guideflandre

  • I have converted the PR into a draft, given that it needs first to unimod package or data to be available.
  • I have updated the github action config to fix the error.
  • I have bumped the version to match the latest Bioc devel

@guideflandre
Copy link
Contributor Author

  • I have converted the PR into a draft, given that it needs first to unimod package or data to be available.

It does not, I have commented out the call to the unimod function. For now this PR simply accepts positional mods in delta mass format.

The unimod parser would convert any annotation style into a delta mass format accepted by calculateFragments, but this can be added in a later PR

Copy link
Member

@sgibb sgibb left a comment

Choose a reason for hiding this comment

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

Thanks for this PR!
Is there a way to express fixed modifications with delta mass? For usability I would prefer to have just one way to express modifications (instead of at least 3, variable, fixed and parsed ...).

Please have a look at #17 (IMHO these both PRs are almost identical).

131.04049, 147.06841, 97.05276, 87.03203,
101.04768, 186.07931, 163.06333, 99.06841,
149.03000, 237.29945),
150.95363, 237.14772),
Copy link
Member

Choose a reason for hiding this comment

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

It would be great if we could replace this function with data from the unimod package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the content is redundant and fits better within the unimod package. Shall we wait for it (unimod/PTMatch) to be ready for Bioconductor before applying these changes ?

@guideflandre guideflandre marked this pull request as ready for review January 2, 2026 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants