matchMz, CompareMassParam #64

andreavicini · 2022-03-02T11:07:02Z

Hi, this PR contains a first implementation of the matchMz, CompareMassParam that we discussed in issue #56. There are a few things I'm not very sure of and I will try to add specific question on the code.

Merge branch 'master' into andrea # Conflicts: # NAMESPACE

andreavicini · 2022-03-02T11:09:48Z

R/matchMz.R

+         slots = c(
+           queryAdducts = "adductClass",
+           targetAdducts = "adductClass"),
+         contains = "MzParam",


Is this ok or should I just inherit from "Param"? The name might be misleading but actually "MzParam" would be ok also for mass matching. We could even inherit from "Mass2MzParam" adding only the slot "queryAdducts" (and possibly rename slot "adduct" to "targetAdducts" in "Mass2MzParam"). What do you think would be best?

I think it would be good to rename the adduct slot in Mass2MzParam to targetAdduct and also add a queryAdduct slot to Mass2MzParam (even if we don't use it for now). Then I would rename the CompareMassParam class maybe to Mz2MassParam (see also discussion issue #56 ) and make that extending Mass2MzParam.

I'm actually even wondering if we shouldn't also add a slot FUN to that class with default value @FUN = mass2mz for Mass2MzParam and @FUN = mz2mass for Mz2MassParam - the user should not have the possibility to set this slot (i.e. it should not be included as a parameter in the Mass2MzParam or Mz2MassParam functions...

R/matchMz.R

andreavicini · 2022-03-02T11:12:53Z

R/matchMz.R

+                                          target = target_mass,
+                                          tolerance = param@tolerance,
+                                          ppm = param@ppm)
+              if (nrow(matches[[i]]))


or create separate columns for query and target adducts?

It would be helpful to know which adducts matched.

Sorry, I referenced one line above the correct one. Information on matched adducts is added in the same way as in PR #58.

I think it might be more helpful to have them in separate columns, i.e. "query_adduct" and "target_adduct".

R/matchMz.R

jorainer

Thanks Andrea for implementing! While going through the code I realized (like you did) that there is room to avoid redundancy and code duplication. Maybe we should first discuss some suggested changes also in issue #56 and then adapt this PR.

R/matchMz.R

jorainer · 2022-03-02T14:35:42Z

R/matchMz.R

+                                          target = target_mass,
+                                          tolerance = param@tolerance,
+                                          ppm = param@ppm)
+              if (nrow(matches[[i]]))


I think it might be more helpful to have them in separate columns, i.e. "query_adduct" and "target_adduct".

R/matchMz.R

jorainer · 2022-03-02T15:03:21Z

R/matchMz.R

+         slots = c(
+           queryAdducts = "adductClass",
+           targetAdducts = "adductClass"),
+         contains = "MzParam",


I think it would be good to rename the adduct slot in Mass2MzParam to targetAdduct and also add a queryAdduct slot to Mass2MzParam (even if we don't use it for now). Then I would rename the CompareMassParam class maybe to Mz2MassParam (see also discussion issue #56 ) and make that extending Mass2MzParam.

I'm actually even wondering if we shouldn't also add a slot FUN to that class with default value @FUN = mass2mz for Mass2MzParam and @FUN = mz2mass for Mz2MassParam - the user should not have the possibility to set this slot (i.e. it should not be included as a parameter in the Mass2MzParam or Mz2MassParam functions...

andreavicini · 2022-03-11T07:42:27Z

R/matchMz.R

-                    metadata = list(param = param))
+              target_mz <- .mass_to_mz_df(target, param@targetAdducts)
+              matches <- do.call(
+                  rbind, bpmapply(seq_along(query), query, FUN = .getMatches,


Was there a reason why for this function we left bpmapply instead of a for loop like in all the other functions?

Hm, I don't know. bpmapply would make only sense if .getMatches is slow. Question is also if for loop or simple lapply?

OK for me to convert to for loop if it simplifies the code (and is not slower).

If I remember correctly the first implementation had lapply then we moved to bpmapply and finally to the for loop (which is used in all the other cases except this one). To be sure maybe I can check which one is best.

andreavicini · 2022-03-11T07:45:05Z

R/matchMz.R

@@ -430,18 +560,18 @@ setMethod("matchMz",
                        target = "numeric",
                        param = "MzParam"),
          function(query, target, param, BPPARAM = SerialParam()) {


And here, like in all the other function where bpmapply is not used maybe BPPARAM is not needed

yes, you're right.

jorainer

Quite a comprehensive PR ;) Looks good Andrea, I have some minor change requests but the rest really looks great.

jorainer · 2022-03-11T09:44:03Z

R/matchMz.R

+          signature = c(query = "numeric",
+                        target = "data.frameOrSimilar",
+                        param = "ValueParam"),
+          function(query, target, param, valueColname,


what happens when valueColname is not defined? do you have a unit test for that?

You are right. Actually since you suggested not to export ValueParam I thought also matchMz, ValueParam wouldn't be used directly by the user but maybe like you said it wouldn't be bad to export it even though maybe it's not very likely that the user wants to match other things than mz and mass. If valueColname is not defined I think an error should be thrown (because there's not default value for it) but I could add a condition if (missing(valueColname)) stop("ValueParam has to be provided") and check this in a unit test.

Or, if we support only column names (i.e. character) a cleaner solution would be to have valueParam = character() in the function definition and check if length(valueParam) > 0.

jorainer · 2022-03-11T09:45:44Z

R/matchMz.R

-                    metadata = list(param = param))
+              target_mz <- .mass_to_mz_df(target, param@targetAdducts)
+              matches <- do.call(
+                  rbind, bpmapply(seq_along(query), query, FUN = .getMatches,


OK for me to convert to for loop if it simplifies the code (and is not slower).

jorainer · 2022-03-11T09:49:33Z

R/matchMz.R

+          signature = c(query = "numeric",
+                        target = "numeric",
+                        param = "Mz2MassParam"),
+          function(query, target, param, BPPARAM = SerialParam()) {


remove the BPPARAM if you're using a for loop.

jorainer · 2022-03-11T09:54:39Z

R/matchMz.R

-            Matched(query = query, target = target,
-                    matches = do.call(rbind, res),
-                    metadata = list(param = param))
+              target_mz <- data.frame(index = seq_along(target),


This looks identical to matchMz, numeric, numeric, ValueParam - so I think you can completely remove this method, since MzParam is also inheriting from ValueParam.

You are right! Thanks!

jorainer · 2022-03-11T09:55:33Z

R/matchMz.R

+          signature = c(query = "numeric",
+                        target = "numeric",
+                        param = "ValueParam"),
+          function(query, target, param, BPPARAM = SerialParam()) {


drop the BPPARAM here.

R/matchMz.R

andreavicini · 2022-03-16T14:45:06Z

Hi, I’ve pushed the requested updates and also tried to correct documentation. I would still have the following questions. SummarizedExperiment for query should be supported for all the various matching approaches/parameter objects? I noticed that at the moment it should work only for those using also retention time for the match. Should also a Mz2MassRtParam be added?

michaelwitting · 2022-03-17T07:46:42Z

Yes, we would need a Mz2MassRtParam for matching between ion modes and use rtime as additional filter. Here two SummarizedExperiment can be also the input, one as query and one as target

andreavicini · 2022-03-22T08:56:45Z

I have added matchMz, Mz2MassRtParam but SummarizedExperiment is not yet supported and I would like to ask your opinion on that. If besides query we allow also target to be SummarizedExperiment maybe we should create 3 different classes inheriting from Matched. At the moment we have MatchedSummarizedExperiment where only query is expected to be SummarizedExperiment and maybe we should add another 2 for the cases when only target is SummarizedExperiment or both are. Alternatively since the only thing that changes between these is whether or not the rowData is called on query and/or target, maybe we could define one function that depending on the type of query/target returns identity or rowData and use only the Matched class with that. What do you think would be best?

michaelwitting · 2022-03-22T09:50:34Z

I would go for the solution using only the rowData and have simple Matched object as results containing the rowData of both, query and target

andreavicini · 2022-03-22T14:48:56Z

I thought we wanted to have the slot @query of the matched object to contain the object passed via matchMz parameter query but yes as you said we could also could fill slot @query with rowData(query). In that case then we wouldn’t need the MatchedSummarizedExperiment class or similar any more

michaelwitting · 2022-03-22T17:04:53Z

Maybe we can get feedback from @jorainer ? I didn't use it so far with a SummarizedExperiment. How would the return data look like when I'm calling matchedData is it the complete data with intensities or only everything in rowData of query and target?

andreavicini · 2022-03-23T08:11:12Z

Calling matchedData on a MatchedSummarizedExperiment returns by default all columns in rowData(query) and target (plus variables associated to the matches such as score)

michaelwitting · 2022-03-23T10:57:51Z

Okay. That is fine for me. I wasn't sure about the return of matchedData, if it would have returned everything that would have been a lot. I think it makes sense then to have a MatchSummarizedExperiment

jorainer · 2022-03-23T15:43:55Z

Sorry, I'm a little late on the discussion here. I think @query or @target should always contain the full input object (e.g. the full SummarizedExperiment) but, as @andreavicini pointed out, matching is done on rowData and hence rowData is also presented with the matchedData function.

I haven't thought we will need to support also a target = SummarizedExperiment, but maybe that's good to have - for completeness.

jorainer · 2022-03-23T15:49:25Z

Actually, I would prefer to first merge this PR before adding support for SummarizedExperiment @andreavicini - otherwise the PR becomes rather lengthy and hard to read.

andreavicini · 2022-03-23T16:00:28Z

I agree. I too had the impression that this PR maybe is getting rather lenghty.

jorainer

I'm OK with this PR. Let's merge and then work separately on the SummarizedExperiment.

andreavicini added 4 commits February 23, 2022 16:15

correction

41b6bc7

matchMz, CompareMassParam

ec7f6a0

merge master

496ea0b

Merge branch 'master' into andrea # Conflicts: # NAMESPACE

version and NEWS.md update

9245f96

andreavicini commented Mar 2, 2022

View reviewed changes

R/matchMz.R Outdated Show resolved Hide resolved

andreavicini commented Mar 2, 2022

View reviewed changes

R/matchMz.R Show resolved Hide resolved

andreavicini marked this pull request as ready for review March 2, 2022 11:31

andreavicini requested review from jorainer and michaelwitting March 2, 2022 11:32

jorainer mentioned this pull request Mar 2, 2022

matchIonMode function? #56

Closed

jorainer requested changes Mar 2, 2022

View reviewed changes

VaueParam and other suggested changes

0c76791

andreavicini commented Mar 11, 2022

View reviewed changes

andreavicini requested a review from jorainer March 11, 2022 07:45

jorainer requested changes Mar 11, 2022

View reviewed changes

updates: code and documentation

c3e9e27

andreavicini requested a review from jorainer March 16, 2022 14:45

matchMz, Mz2MassRtParam

1b4e836

jorainer approved these changes Mar 23, 2022

View reviewed changes

jorainer merged commit 193c065 into master Mar 23, 2022

matchMz, CompareMassParam #64

matchMz, CompareMassParam #64

Conversation

andreavicini commented Mar 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreavicini Mar 2, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorainer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorainer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreavicini commented Mar 16, 2022

michaelwitting commented Mar 17, 2022

andreavicini commented Mar 22, 2022

michaelwitting commented Mar 22, 2022

andreavicini commented Mar 22, 2022

michaelwitting commented Mar 22, 2022

andreavicini commented Mar 23, 2022

michaelwitting commented Mar 23, 2022

jorainer commented Mar 23, 2022

jorainer commented Mar 23, 2022

andreavicini commented Mar 23, 2022

jorainer left a comment

Choose a reason for hiding this comment

andreavicini Mar 2, 2022 •

edited

Loading