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

matchIonMode function? #56

Closed
michaelwitting opened this issue Jan 21, 2022 · 19 comments
Closed

matchIonMode function? #56

michaelwitting opened this issue Jan 21, 2022 · 19 comments

Comments

@michaelwitting
Copy link
Collaborator

What about having a matchIonMode() function? I have sometimes the case to match between ion modes and the results would be perfectly fitted for the Matched object, e.g. in negative mode you have detected a [M-H]- and in positive the respective [M+H]+ and [M+Na]+. The function can accept x and y and xAdducts and yAdducts, perform mz2mass with that and match the neutral mass. Additionally with retention time matching, if required.
Thoughts?

@jorainer
Copy link
Member

Could you maybe explain that with a use case describing also the input data that you have? Is it then features from positive and negative polarity?

@michaelwitting
Copy link
Collaborator Author

Yes, the idea is to have either two vectors of m/z values from positive and negative ion mode and match them. Alternatively, two feature tables either as data.frame or SummarizedExperiment can be matched, optionally also using retention time as additional filter. Having this you can start comparing data from positive and negative ion mode and e.g. check if you detected the same features and if annotation in the both modes yield the same or similar results

@jorainer
Copy link
Member

jorainer commented Feb 3, 2022

I like the idea - but that could be even generalized to a CompareMassParam I think. So CompareMassParam could have parameters queryAdduct and targetAdduct as well as ppm and tolerance. You could then call:

res <- matchMz(qry, trgt, CompareMassParam(queryAdduct = c("[M+H]+", "[M+Na]+"), targetAdduct = "[M-H]-", ppm = 10))

with qry and trgt being m/z values form e.g. features in positive and negative polarity. The function would then first convert the m/z from qry to masses based on queryAdduct and the m/z from trgt to masses based on targetAdduct and these are then compared. A comparison with itself, i.e. if qry is passed with parameter query and with parameter target values/features could be grouped if they represent different adducts of the same compound.

The matching that considers also rt could then maybe done in a second step with a CompareMassRtParam...

I like the idea. Do you want to give it a try or should I ask Andrea to implement it?

@michaelwitting
Copy link
Collaborator Author

Agreed, we can rename everything to matchMass and the ionmode matching is special case. I already made a simple implementation for the ion mode matching.
#58
I will change naming, etc accordingly.

@michaelwitting
Copy link
Collaborator Author

PS: I would like to have a dedicated functions called matchMass instead of having it as special case of matchMz. Naming might lead to confusion here, when the function name says you compare m/z, but reality its neutral masses.
On the other side, we have adducts as arguments, which might be confusing when you want to compare neutral masses.

Tricky situation. Ideas? @andreavicini maybe?

@jorainer
Copy link
Member

jorainer commented Feb 4, 2022

Hm, yes. But it's similar with matchMz,Mass2MzParam - there we convert masses to m/z prior comparison. Maybe we should have called the function matchValues from the very beginning...

The issue with matchMass would then again be that this would work only if both query and target are masses - if one of the two is an m/z and the other a mass, do we then use matchMz or matchMass? :)

@michaelwitting
Copy link
Collaborator Author

Ahh.... too many options. ;-)
Okay, then let's have a new parameter object and please ignore my draft PR. @andreavicini can have a look at my code maybe and adopt it into matchMz.

@andreavicini
Copy link
Collaborator

Yes, I can do that! I agree that maybe it’s better to have only one name for the function

@jorainer
Copy link
Member

jorainer commented Feb 7, 2022

OK, then @andreavicini , please add that functionality maybe checking if you can take something from @michaelwitting's PR #58

@jorainer
Copy link
Member

jorainer commented Feb 7, 2022

That OK for you @michaelwitting ?

@michaelwitting
Copy link
Collaborator Author

Totally fine...

@jorainer
Copy link
Member

jorainer commented Mar 2, 2022

When reading the PR #64 I got confused:

The idea is now that the user can provide m/z values and these are converted to masses and then compared, right? So, it is different from the already implemented Mass2MzParam, right? Mass2MzParam converts target masses to m/z values and compares them against query m/z values.

Here we want to do the comparison on the mass level, not the m/z level (which can yield different results because of the ppm).

So, here, we would maybe better call the parameter Mz2MassParam which allows to define queryAdduct and targetAdduct based on which the query and target values get converted with mz2mass to exact masses and then compared.

The Mass2MzParam does the opposite: convert target masses with mass2mz to m/z values and compares these to the query m/z values - in a later step we might change that also to support queryAdduct and targetAdduct...

Does this make sense @michaelwitting and @andreavicini?

@andreavicini
Copy link
Collaborator

Yes, it does more or less the opposite. The difference is that both query and target values get converted before the match while for Mass2MzParam only target does (so "it would be like a TargetMass2MzParam")

@michaelwitting
Copy link
Collaborator Author

I don't know if we really need queryAdduct and targetAdduct in Mass2MzParam, normally you want to compare here m/z values from similar adducts. Mz2MassParam for me is a bit different, since it also allows to match between different ion modes. Therefore we need the different adduct definitions.
To keep maximum flexibility, we can have queryAdduct and targetAdduct in all methods. If a user only suggests a queryAdduct and targetAdduct is NA they will be the same. At least that would be my suggestion.

@andreavicini
Copy link
Collaborator

andreavicini commented Mar 3, 2022

I’m not sure I understand correctly. Do you mean to have queryAdducts and targetAdducts in both Mass2MzParam and Mz2MassParam? And to set them equal to each other if the user provides only a value for queryAdducts?

I think we could have a more general code but I’m not sure it would became less clear or would be helpful for the user. For example would it be useful in some circumstance to match values other than m/z in query and target (e.g. mass or even something else)? In that case we could create a ValueParam with only ppm and tolerance slots.
Without considering matching that involve retention time now we have:

MzParam: matches m/z values in query and target
Mass2MzParam: matches query m/z to m/z obtained transforming target masses.
Mz2MassParam: matches masses obtained transforming query and target m/z.

Is there any other useful use case (e.g. matching m/zs obtained transforming query and target masses? or other combinations?) that could be useful?

If not why would you add queryAdducts also to Mass2MzParam? Also regarding the comment in PR #64 I think that using an additional @FUN parameter could mean that we could have a single converting function instead of 2 (.mz_to_mass_df and .mass_to_mz_df). I'm also ok with making Mz2MassParam inherit from Mass2MzParam (although maybe writing only the function for query and target "numeric" wouldn't be enough any more with this kind of inheritance but I don't know if we want to exploit that).

@michaelwitting
Copy link
Collaborator Author

I agree with this list @andreavicini :

MzParam: matches m/z values in query and target
Mass2MzParam: matches query m/z to m/z obtained transforming target masses.
Mz2MassParam: matches masses obtained transforming query and target m/z.

Having this now in front of me, I think we need queryAdducts and targetAdducts only in the Mz2MassParam, for the others adducts is sufficient.

@jorainer
Copy link
Member

jorainer commented Mar 7, 2022

I agree with both of you @andreavicini and @michaelwitting . So, maybe the following would make sense:

  • ValueParam: with slots @ppm and @tolerance - but we don't export that class.
  • MzParam: inherits ValueParam, we export that.
  • Mass2MzParam: inherit ValueParam, add slot targetAdducts (or keep it adducts - I would just find it more convenient to have the slot named targetAdducts, while in the Mass2MzParam constructor function I would keep parameter adducts - to be backward compatible...)
  • Mz2MassParam: inherit ValueParam, add slot targetAdducts and queryAdducts.

@andreavicini
Copy link
Collaborator

That seems good to me! So we don’t export ValueParam because we don’t expect the user to perform matches based on other values than m/z at the moment, right?

@michaelwitting
Copy link
Collaborator Author

Seems fine to me. No, it will be only m/z. RT and CCS will be always on top of that.

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

No branches or pull requests

3 participants