Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow changes to molecule name (or anything that doesn't change its mass) without losing the association to any existing chromatograms. #2570

Conversation

bspratt
Copy link
Member

@bspratt bspratt commented Apr 22, 2023

Users expect that just changing the name won't make chromatograms go away.

Observed by Brendan while watching a small molecule tutorial being presented in Dortmund

…ass) without losing the association to any existing chromatograms.

Observed by Brendan while watching a small molecule tutorial being presented in Dortmund
@nickshulman
Copy link
Contributor

We should think about how this work meshes with PR #2450.

In PR #2450 the serialized string (Chromatogram Text Id) in the .skyd file gets replaced by something that gets serialized to a protocol buffer, and which is a "ChromatogramGroupId":
ChromatogramGroupId.cs

@bspratt
Copy link
Member Author

bspratt commented Apr 24, 2023

@nickshulman Should be no big deal to merge the two. My changes are relatively lightweight and largely superseded by yours - as you suspected, pretty much everywhere that I've changed from using "ModifiedTarget" to "ChromatogramTarget" you've already changed to "ChromatogramGroupId".

Really the only thing that needs to survive from my work is using this:
Peptide.OriginalMoleculeTarget ?? ModifiedTarget
instead of peptideDocNode.ModifiedTarget in ChromatogramGroupId.ForPeptide().

So the changes I'd need to make to your work are even smaller than the ones I'd need to make to the current code.

I can branch your branch if you'd like to see what this would look like.

If your code is likely to be integrated any time soon then we should probably save the churn and just put my stuff into yours and abandon my branch.

…k/20230421_allow_molecule_name_change_without_chromatogram_loss
bspratt added a commit that referenced this pull request Apr 25, 2023
…ge_without_chromatogram_loss (PR #2570 "Allow changes to molecule name (or anything that doesn't change its mass) without losing the association to any existing chromatograms.") overlaid on Nick's branch for custom precursor filtering
@bspratt
Copy link
Member Author

bspratt commented Apr 25, 2023

@nickshulman see my PR 2574 for details.

@nickshulman
Copy link
Contributor

I wonder whether it would be better to fix this by making it so that when looking for chromatograms in the .skyd file, if an exact match on the TextId is not found, then it tries to find the TextId which most closely matches.
The logic for finding chromatograms in the .skyd file is in "ChromatogramCache.ChromatogramIndexesMatching".

One of the problems with changing it so that chromatograms are identified by the original name of the Molecule is that you would then have weird scenarios where two molecules might have a conflict in the .skyd file because two molecules which currently have different names used to have the same name.

Maybe Skyline should not remember the original names of molecules if there are no chromatograms in the document. Also, the original names would be forgotten if the user told Skyline to reimport all chromatograms.

@bspratt
Copy link
Member Author

bspratt commented Apr 25, 2023

"find the TextId which most closely matches" sounds pretty wiggly. If the user manages to tweak a molecule such that its name and formula/mass and child ion adduct and IM match more than one chromatogram, does it really matter which chromatogram gets referenced? Don't we already deal with this ambiguity, given that a molecule (or peptide) can exist in more than one place in the targets tree?

Maybe Skyline should not remember the original names of molecules if there are no chromatograms in the document.

We could do that - it's not useful information in that case.

Also, the original names would be forgotten if the user told Skyline to reimport all chromatograms.

That would be kind of weird, forcing an identity change based on reimport.

Copy link
Contributor

@nickshulman nickshulman left a comment

Choose a reason for hiding this comment

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

Looks good to me.

…t_chromatogram_loss' of https://github.com/ProteoWizard/pwiz into Skyline/work/20230421_allow_molecule_name_change_without_chromatogram_loss
@bspratt bspratt merged commit 618ae79 into master May 3, 2023
12 checks passed
@bspratt bspratt deleted the Skyline/work/20230421_allow_molecule_name_change_without_chromatogram_loss branch May 3, 2023 17:46
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.

None yet

2 participants