filterMatches, ScoreThresholdParam #87
Conversation
Merge branch 'master' into andrea # Conflicts: # R/Matched.R
There was a problem hiding this comment.
Very nice Andrea! I have however one request: maybe better to use a slot/parameter column instead of filterScoreRt that allows to specify the column in @matches which should be used for the filtering. That would be more flexible and would allow us in future also to use this param even if we add additional (different) score columns.
| @@ -148,6 +156,10 @@ | |||
| #' - `whichQuery` returns an `integer` with the indices of the elements in | |||
| #' *query* that match at least one element in *target*. | |||
| #' | |||
| #' @param above for `ScoreThresholdParam` : `logical(1)` specifying whether | |||
There was a problem hiding this comment.
Maybe it would be good to describe whether above (or below) means > or >=.
R/Matched.R
Outdated
| @@ -156,6 +168,12 @@ | |||
| #' `decreasing = FALSE`. | |||
| #' | |||
| #' @param drop for `[`: ignored. | |||
| #' | |||
| #' @param filterScoreRt for `ScoreThresholdParam` : `logical(1)` specifying | |||
There was a problem hiding this comment.
I think it might be better to replace this parameter with a parameter column = "score" which allows to define the column in the matches data.frame that should be used for the filtering. That would allow us also to use this param later for other columns (scores) we might add.
R/Matched.R
Outdated
| slots = c( | ||
| threshold = "numeric", | ||
| above = "logical", | ||
| filterScoreRt = "logical"), |
There was a problem hiding this comment.
as described above, I would replace this slot with column that has as default "score".
R/Matched.R
Outdated
| #' @export | ||
| setMethod("filterMatches", c("Matched", "ScoreThresholdParam"), | ||
| function (object, param, ...) { | ||
| scorename <- ifelse(param@filterScoreRt, "score_rt", "score") |
There was a problem hiding this comment.
After changing to column you would need to check if the @matches has a column called column and throw an error otherwise.
|
I agree with you and I should have updated accordingly. I was not sure if the user would know the name of the "score variables" available in a object. Maybe we could return such names with a simple function (assuming they are the names in |
|
@andreavicini , regarding your question with the function to return score column names - I think that's a great idea! Question is just how to name that function - should it be |
|
@jorainer, I'm fine with both or could also be |
|
Hm, yes, I think we do also add other information into the |
|
Yes, you are right about the adduct columns (don't know why I didn't think of that 😅). But now I'm thinking: if we want a function that returns only score columns in |
|
Unrelated question: do you think it would ever be helpful to filter based on character variables in |
|
For the former, we could have a simple For the second question, yes, I think it would be helpful to filter also based on e.g. |
filterMatches, ScoreThresholdParamto perform filtering the matches based on a threshold for the"score"variable. Also filtering based on"score_rt"is possible withfilterScoreRt = TRUE(not sure if that's the best way or, for example, we could also use a parameterscorenamewith the name of the score the filtering will use and default value "score"?)