refactor: address Phili's comments

.github/workflows/check-bioc.yml

@@ -155,9 +155,6 @@ jobs:
           ## For installing usethis's dependency gert
           brew install libgit2
 
-          ## Python
-          ## brew install [email protected]
-
       - name: Install Windows system dependencies
         if: runner.os == 'Windows'
         run: |

DESCRIPTION

@@ -58,6 +58,7 @@ Suggests:
     MsBackendMgf,
     msdata,
     mzR,
+    knitr,
     BiocStyle
 License: Artistic-2.0
 BugReports: https://github.com/RforMassSpectrometry/SpectriPy/issues

R/compareSpectriPy.R

@@ -5,7 +5,7 @@
 #' @description
 #'
 #' The `compareSpectriPy()` function allows to calculate spectral similarity
-#' scores using the `calculate_scores function` of the Python
+#' scores using the `calculate_scores()` function of the Python
 #' [matchms.similarity](https://matchms.readthedocs.io/en/latest/api/matchms.similarity.html).
 #' module.
 #'
@@ -145,7 +145,7 @@ setGeneric("compareSpectriPy", function(x, y, param, ...) {
     standardGeneric("compareSpectriPy")
 })
 
-setClass("CosineGreedyParam",
+setClass("CosineGreedy",
     slots = c(
         tolerance = "numeric", mzPower = "numeric", intensityPower = "numeric"
     ),
@@ -164,10 +164,10 @@ setClass("CosineGreedyParam",
         msg
     }
 )
-setClass("CosineHungarianParam", contains = "CosineGreedyParam")
-setClass("ModifiedCosineParam", contains = "CosineGreedyParam")
-setClass("NeutralLossesCosineParam",
-    contains = "CosineGreedyParam",
+setClass("CosineHungarian", contains = "CosineGreedy")
+setClass("ModifiedCosine", contains = "CosineGreedy")
+setClass("NeutralLossesCosine",
+    contains = "CosineGreedy",
     slots = c(ignorePeaksAbovePrecursor = "logical"),
     prototype = prototype(ignorePeaksAbovePrecursor = TRUE),
     validity = function(object) {
@@ -179,7 +179,7 @@ setClass("NeutralLossesCosineParam",
         msg
     }
 )
-setClass("FingerprintSimilarityParam", contains = "CosineGreedyParam",
+setClass("FingerprintSimilarity", contains = "CosineGreedy",
          slots = c(similarityMeasure = "character"),
          prototype = prototype(similarityMeasure = "jaccard"))
 
@@ -190,7 +190,7 @@ setClass("FingerprintSimilarityParam", contains = "CosineGreedyParam",
 #' @export
 CosineGreedy <- function(tolerance = 0.1, mz_power = 0.0,
                          intensity_power = 1.0) {
-    new("CosineGreedyParam",
+    new("CosineGreedy",
         tolerance = as.numeric(tolerance),
         mzPower = as.numeric(mz_power),
         intensityPower = as.numeric(intensity_power)
@@ -202,7 +202,7 @@ CosineGreedy <- function(tolerance = 0.1, mz_power = 0.0,
 #' @export
 CosineHungarian <- function(tolerance = 0.1, mz_power = 0.0,
                             intensity_power = 1.0) {
-    new("CosineHungarianParam",
+    new("CosineHungarian",
         tolerance = as.numeric(tolerance),
         mzPower = as.numeric(mz_power),
         intensityPower = as.numeric(intensity_power)
@@ -214,7 +214,7 @@ CosineHungarian <- function(tolerance = 0.1, mz_power = 0.0,
 #' @export
 ModifiedCosine <- function(tolerance = 0.1, mz_power = 0.0,
                                 intensity_power = 1.0) {
-    new("ModifiedCosineParam",
+    new("ModifiedCosine",
         tolerance = as.numeric(tolerance),
         mzPower = as.numeric(mz_power),
         intensityPower = as.numeric(intensity_power)
@@ -227,7 +227,7 @@ ModifiedCosine <- function(tolerance = 0.1, mz_power = 0.0,
 NeutralLossesCosine <- function(tolerance = 0.1, mz_power = 0.0,
                                 intensity_power = 1.0,
                                 ignore_peaks_above_precursor = TRUE) {
-    new("NeutralLossesCosineParam",
+    new("NeutralLossesCosine",
         tolerance = as.numeric(tolerance),
         mzPower = as.numeric(mz_power),
         intensityPower = as.numeric(intensity_power),
@@ -239,7 +239,7 @@ NeutralLossesCosine <- function(tolerance = 0.1, mz_power = 0.0,
 ## #'
 ## #' @export
 ## FingerprintSimilarity <- function(similarity_measure = "jaccard") {
-##     new("FingerprintSimilarityParam",
+##     new("FingerprintSimilarity",
 ##         similarityMeasure = similarity_measure
 ##         )
 ## }
@@ -249,7 +249,7 @@ NeutralLossesCosine <- function(tolerance = 0.1, mz_power = 0.0,
 #' @exportMethod compareSpectriPy
 setMethod(
     "compareSpectriPy",
-    signature = c(x = "Spectra", y = "Spectra", param = "CosineGreedyParam"),
+    signature = c(x = "Spectra", y = "Spectra", param = "CosineGreedy"),
     function(x, y, param, ...) {
         .compare_spectra_python(x, y, param)
     }
@@ -258,7 +258,7 @@ setMethod(
 #' @rdname compareSpectriPy
 setMethod(
     "compareSpectriPy",
-    signature = c(x = "Spectra", y = "missing", param = "CosineGreedyParam"),
+    signature = c(x = "Spectra", y = "missing", param = "CosineGreedy"),
     function(x, y, param, ...) {
         .compare_spectra_python(x, y = NULL, param)
     }
@@ -267,26 +267,29 @@ setMethod(
 #' Method to construct a python function name for a param object.
 #'
 #' @noRd
-setGeneric("py_fun", function(x, ...) {
+setGeneric("py_fun", function(object, ...) {
     standardGeneric("py_fun")
 })
-setMethod("py_fun", "CosineGreedyParam", function(x) {
-    matchms_similarity$CosineGreedy(x@tolerance, x@mzPower, x@intensityPower)
+setMethod("py_fun", "CosineGreedy", function(object) {
+    matchms_similarity$CosineGreedy(object@tolerance, object@mzPower,
+                                    object@intensityPower)
 })
-setMethod("py_fun", "CosineHungarianParam", function(x) {
-    matchms_similarity$CosineHungarian(x@tolerance, x@mzPower, x@intensityPower)
+setMethod("py_fun", "CosineHungarian", function(object) {
+    matchms_similarity$CosineHungarian(object@tolerance, object@mzPower,
+                                       object@intensityPower)
 })
-setMethod("py_fun", "ModifiedCosineParam", function(x) {
-    matchms_similarity$ModifiedCosine(x@tolerance, x@mzPower, x@intensityPower)
+setMethod("py_fun", "ModifiedCosine", function(object) {
+    matchms_similarity$ModifiedCosine(object@tolerance, object@mzPower,
+                                      object@intensityPower)
 })
-setMethod("py_fun", "NeutralLossesCosineParam", function(x) {
-    matchms_similarity$NeutralLossesCosine(x@tolerance, x@mzPower,
-                                           x@intensityPower,
-                                           x@ignorePeaksAbovePrecursor)
+setMethod("py_fun", "NeutralLossesCosine", function(object) {
+    matchms_similarity$NeutralLossesCosine(object@tolerance, object@mzPower,
+                                           object@intensityPower,
+                                           object@ignorePeaksAbovePrecursor)
 })
 
 .fun_name <- function(x) {
-    sub("Param$", "", class(x)[1L])
+    class(x)[1L]
 }
 
 #' Internal function to calculate similarities with Python's matchms. `Spectra`

R/conversion.R

@@ -64,7 +64,7 @@
 #' - `defaultSpectraVariableMapping()`: returns the *default* mapping between
 #'   spectra variables and *matchms* metadata names.
 #'
-#' - `spectraVariableMapping()`: returns the currenctly defined spectra
+#' - `spectraVariableMapping()`: returns the currently defined spectra
 #'   variable mapping as a named character vector, with names representing the
 #'   names of the spectra variables in R and elements the respective names
 #'   of the spectra metadata in Python. Use [Spectra::spectraVariables()] on

R/filterSpectriPy.R

@@ -253,28 +253,28 @@ setMethod(
 #' @importFrom reticulate py_dict
 #'
 #' @noRd
-setMethod("py_fun", "select_by_intensity", function(x) {
+setMethod("py_fun", "select_by_intensity", function(object) {
     matchms_filtering$SpectrumProcessor$create_partial_function(
         matchms_filtering$select_by_intensity,
         py_dict(c("intensity_from", "intensity_to"),
-                c(x@intensity_from, x@intensity_to)))
+                c(object@intensity_from, object@intensity_to)))
 })
 
-setMethod("py_fun", "select_by_mz", function(x) {
+setMethod("py_fun", "select_by_mz", function(object) {
     matchms_filtering$SpectrumProcessor$create_partial_function(
         matchms_filtering$select_by_mz,
         py_dict(c("mz_from", "mz_to"),
-                c(x@mz_from, x@mz_to)))
+                c(object@mz_from, object@mz_to)))
 })
 
-setMethod("py_fun", "remove_peaks_around_precursor_mz", function(x) {
+setMethod("py_fun", "remove_peaks_around_precursor_mz", function(object) {
     matchms_filtering$SpectrumProcessor$create_partial_function(
         matchms_filtering$remove_peaks_around_precursor_mz,
         py_dict(c("mz_tolerance"),
-                c(x@mz_tolerance)))
+                c(object@mz_tolerance)))
 })
 
-setMethod("py_fun", "normalize_intensities", function(x) {
+setMethod("py_fun", "normalize_intensities", function(object) {
     matchms_filtering$SpectrumProcessor$create_partial_function(
         matchms_filtering$normalize_intensities)
 })

tests/testthat/test_compareSpectriPy.R

@@ -34,55 +34,55 @@ test_that("param constructors work", {
         ignore_peaks_above_precursor = c(TRUE, FALSE)), "length 1")
 })
 
-test_that("CosineGreedyParam constructor works", {
+test_that("CosineGreedy constructor works", {
     res <- CosineGreedy(tolerance = 5)
-    expect_s4_class(res, "CosineGreedyParam")
+    expect_s4_class(res, "CosineGreedy")
     expect_equal(res@tolerance, 5)
 })
 
-test_that("CosineHungarianParam constructor works", {
+test_that("CosineHungarian constructor works", {
     res <- CosineHungarian(intensity_power = 1.3)
-    expect_s4_class(res, "CosineGreedyParam")
+    expect_s4_class(res, "CosineGreedy")
     expect_equal(res@intensityPower, 1.3)
 })
 
-test_that("ModifiedCosineParam constructor works", {
+test_that("ModifiedCosine constructor works", {
     res <- ModifiedCosine(mz_power = 4.3)
-    expect_s4_class(res, "ModifiedCosineParam")
+    expect_s4_class(res, "ModifiedCosine")
     expect_equal(res@mzPower, 4.3)
 })
 
-test_that("NeutralLossesCosineParam constructor works", {
+test_that("NeutralLossesCosine constructor works", {
     res <- NeutralLossesCosine(ignore_peaks_above_precursor = FALSE)
-    expect_s4_class(res, "NeutralLossesCosineParam")
+    expect_s4_class(res, "NeutralLossesCosine")
     expect_false(res@ignorePeaksAbovePrecursor)
 })
 
 test_that(".fun_name works", {
     a <- CosineGreedy()
-    expect_equal(SpectriPy:::.fun_name(a), "CosineGreedy")
+    expect_equal(.fun_name(a), "CosineGreedy")
     a <- CosineHungarian()
-    expect_equal(SpectriPy:::.fun_name(a), "CosineHungarian")
+    expect_equal(.fun_name(a), "CosineHungarian")
     a <- ModifiedCosine()
-    expect_equal(SpectriPy:::.fun_name(a), "ModifiedCosine")
+    expect_equal(.fun_name(a), "ModifiedCosine")
     a <- NeutralLossesCosine()
-    expect_equal(SpectriPy:::.fun_name(a), "NeutralLossesCosine")
+    expect_equal(.fun_name(a), "NeutralLossesCosine")
 })
 
 test_that("py_fun works", {
-    res <- SpectriPy:::py_fun(CosineGreedy(tolerance = 0.5, mz_power = 0.3,
+    res <- py_fun(CosineGreedy(tolerance = 0.5, mz_power = 0.3,
                                            intensity_power = 0.2))
     expect_equal(class(res)[1L],
                  "matchms.similarity.CosineGreedy.CosineGreedy")
-    res <- SpectriPy:::py_fun(CosineHungarian(tolerance = 0.4, mz_power = 0.3,
+    res <- py_fun(CosineHungarian(tolerance = 0.4, mz_power = 0.3,
                                            intensity_power = 0.2))
     expect_equal(
         class(res)[1L], "matchms.similarity.CosineHungarian.CosineHungarian")
-    res <- SpectriPy:::py_fun(ModifiedCosine(tolerance = 0.1, mz_power = 0.3,
+    res <- py_fun(ModifiedCosine(tolerance = 0.1, mz_power = 0.3,
                                            intensity_power = 0.2))
     expect_equal(
         class(res)[1L], "matchms.similarity.ModifiedCosine.ModifiedCosine")
-    res <- SpectriPy:::py_fun(NeutralLossesCosine(tolerance = 0.1, mz_power = 0.3,
+    res <- py_fun(NeutralLossesCosine(tolerance = 0.1, mz_power = 0.3,
                                                   intensity_power = 0.2,
                                                   ignore_peaks_above_precursor = FALSE))
     expect_equal(
@@ -92,7 +92,7 @@ test_that("py_fun works", {
 
 test_that(".compare_spectra_python works", {
     all <- c(caf, mhd)
-    res <- SpectriPy:::.compare_spectra_python(all, caf, CosineGreedy())
+    res <- .compare_spectra_python(all, caf, CosineGreedy())
     expect_true(nrow(res) == 4)
     expect_true(ncol(res) == 2)
     expect_equal(res[1, 1], 1)
@@ -104,22 +104,22 @@ test_that(".compare_spectra_python works", {
     expect_true(all(diffs < 0.01))
 
     ## only one spectra
-    res <- SpectriPy:::.compare_spectra_python(all, param = CosineGreedy())
-    res_2 <- SpectriPy:::.compare_spectra_python(all, all, param = CosineGreedy())
+    res <- .compare_spectra_python(all, param = CosineGreedy())
+    res_2 <- .compare_spectra_python(all, all, param = CosineGreedy())
     expect_equal(res, res_2)
 
     ## try with empty Spectra
-    res <- SpectriPy:::.compare_spectra_python(all, all[integer()], CosineGreedy())
+    res <- .compare_spectra_python(all, all[integer()], CosineGreedy())
     expect_true(is.numeric(res))
     expect_true(nrow(res) == length(all))
     expect_true(ncol(res) == 0)
 
-    res <- SpectriPy:::.compare_spectra_python(all[integer()], param = CosineGreedy())
+    res <- .compare_spectra_python(all[integer()], param = CosineGreedy())
     expect_true(is.numeric(res))
     expect_true(nrow(res) == 0)
     expect_true(ncol(res) == 0)
 
-    res <- SpectriPy:::.compare_spectra_python(all[integer()], all, CosineGreedy())
+    res <- .compare_spectra_python(all[integer()], all, CosineGreedy())
     expect_true(is.numeric(res))
     expect_true(nrow(res) == 0)
     expect_true(ncol(res) == length(all))
@@ -133,29 +133,6 @@ test_that("compareSpectriPy works", {
     expect_true(ncol(res_all) == length(all))
     expect_equal(diag(res_all), c(1, 1, 1, 1))
 
-    ## Test python call. LLLLLLL
-    ## cl <- basiliskStart(SpectriPy:::matchms_env)
-    ## p <- CosineGreedyParam()
-    ## cmd <- SpectriPy:::python_command(p)
-    ## py$py_x <- rspec_to_pyspec(caf, mapping = c(precursorMz = "precursor_mz"))
-    ## py$py_y <- rspec_to_pyspec(mhd, mapping = c(precursorMz = "precursor_mz"))
-    ## strng <- paste0("import matchms\n",
-    ##                 "from matchms.similarity import CosineGreedy\n",
-    ##                 "res = matchms.calculate_scores(py_x, py_y, CosineGreedy(), is_symmetric = False)\n")
-    ## py_run_string(strng)
-    ## basiliskStop(cl)
-    ## res <- compareSpectriPy(caf, mhd, param = CosineGreedyParam())
-    ## WHY is this not working??? Seems to be some python issue, maybe a bug
-    ## in CosineGreedy?
-    ##
-    ## WORKS res <- compareSpectriPy(caf, caf, param = CosineGreedyParam())
-    ## WORKS res <- compareSpectriPy(mhd, mhd, param = CosineGreedyParam())
-    ## NOT res <- compareSpectriPy(mhd, caf, param = CosineGreedyParam())
-    ## NOT res <- compareSpectriPy(caf, mhd, param = CosineGreedyParam())
-    ## Maybe the issue is with spectra without any similarity (score = 0) and
-    ## having a result matrix with more than 1 column or row.
-    ## Seems to be the case:
-
     ## CosineGreedy
     res <- compareSpectriPy(caf, mhd, param = CosineGreedy())
     expect_true(nrow(res) == 2)