Functionality to convert between R and Python #10

jorainer · 2022-03-08T06:57:03Z

This PR based on the work of @michaelwitting and:

expands the R-Python-R conversion functions to allow specifying which variables to copy and how to map the variable/metadata names between R and Python.
add and expand documentation.
use the basilisk package to ensure the correct Python environment is available to the package (will be automatically installed and bundled within the package - i.e. is independent on any system Python packages).
expand the vignette.
add unit tests for all functions.

- Modify `rspec_to_pyspec` to support `spectraVariableMapping`. - Add `spectraVariableMapping`. - Add unit tests for `rspec_to_pyspec`. - Configure with `basilisk` package.

jorainer · 2022-03-08T06:57:32Z

Maybe also @chufz and @hechth could have a look at this to see if it is OK?

jorainer · 2022-03-08T06:58:14Z

This is related to issue #8

hechth

Parameterized tests would maybe make the testing cleaner, also I'm unsure about the reference parameter - if it is really not needed it shouldn't be there, if it has to be there (even without being used) it should maybe be stated why.

hechth · 2022-03-08T07:27:50Z

DESCRIPTION

-Roxygen: list(markdown=TRUE)
+Encoding: UTF-8
+StagedInstall: no
+SystemRequirements: python (>= 2.7)


I'd stick with python larger 3.5 or so - not sure how well things work in python2

OK, I'll update to 3.5 (or even newer?)

I think 3.5 or 3.7 are reasonable choices

hechth · 2022-03-08T07:28:18Z

R/basilisk.R

+#' @importFrom basilisk BasiliskEnvironment
+matchms_env <- basilisk::BasiliskEnvironment(
+    envname = "matchms_env", pkgname = "SpectriPy",
+    packages = c("matchms==0.14.0"),
+    channels = c("bioconda", "conda-forge")
+)


This is pretty nice!

yep - the basilisk people really did a great work to ensure reproducibility. Will make it much easier to the end user :)

Actually - is matchms version 0.14.0 OK or do you suggest another, more recent, version?

I don't think there is a more recent one at the moment.

hechth · 2022-03-08T07:32:27Z

R/conversion.R

+rspec_to_pyspec <- function(x, mapping = spectraVariableMapping(),
+                            reference = import("matchms"),
+                            BPPARAM = SerialParam(), .check = TRUE) {
+    if (.check && !is(x, "Spectra"))
+        stop("'x' should be a Spectra object.")
+    plist <- spectrapply(x, .single_rspec_to_pyspec, spectraVariables = mapping,
+                         BPPARAM = BPPARAM)
+    r_to_py(unname(plist))
 }


The reference parameter is actually not used

True, reference is not used here, but in .single_rspec_to_pyspec. It contains the reference to the Python Spectrum object, needed for construction. The question is if we can set a global variable that contains this reference during package loading?

At first I also wanted to drop it - but then I realized:

this allows also to use import("matchms", convert = FALSE) to disable automatic conversion of python to R and vice versa, maybe not wrong to disable to speed up things?

Also, in a loop, you can define this reference once, and then in the loop re-use it. Speeds up things.

I would thus keep it (it's anyway for the advanced user).

I think having a function that populates a global variable might be the best way?

I don't think that passing the reference to the imported matchms is a good thing - this implies you could use different versions of matchms in a single session etc., which is not very clean.

But we can figure out how to do this properly in another issue. See #13

hechth · 2022-03-08T07:33:50Z

R/conversion.R

+pyspec_to_rspec <- function(x, mapping = spectraVariableMapping(),
+                            reference = import("matchms"),
+                            BPPARAM = SerialParam(), .check = TRUE) {
+    if (!is(x, "python.builtin.list"))
+        stop("'x' is expected to be a Python list.")
+    x <- py_to_r(x)
+    if (.check && !all(vapply(x, function(z)
+        is(z, "matchms.Spectrum.Spectrum"), logical(1))))
+        stop("'x' is expected to be a Python list of matchms Spectrum objects.")
+    spectra_list <- bplapply(x, .single_pyspec_to_rspec,
+                             spectraVariables = mapping, BPPARAM = BPPARAM)
+    do.call(concatenateSpectra, spectra_list)
+}


Same here, reference parameter is not used -> does import("matchms") by itself have any implications (initialize global variables) if it is called?

Now that you say it - I think I forgot to pass this reference down to the .single_pyspec_to_rspec function! I will fix that.

We just need to reference in reference to the correct environment that was set up by basilisk, I guess the name needs to be changed to matchms_env then, right? so reference = import("matchms_env")

Forget what I said... stupid stuff. The enviroment is loaded at a different point. It's the import of the package...

hechth · 2022-03-08T07:34:53Z

R/conversion.R

+.single_pyspec_to_rspec <- function(x,
+                                    spectraVariables = spectraVariableMapping(),
+                                    reference = import("matchms")) {
+    plist <- x$metadata
+    vars <- spectraVariables[spectraVariables %in% names(plist)]
+    if (length(vars)) {
+        rlist <- lapply(vars, function(z) plist[z])
+        ## Drop NULL variables.
+        spd <- DataFrame(rlist[lengths(rlist) > 0])
+        if (!nrow(spd))
+            spd <- DataFrame(msLevel = NA_integer_)
+    } else
+        spd <- DataFrame(msLevel = NA_integer_)
+    spd$mz <- NumericList(as.numeric(x$peaks$mz), compress = FALSE)
+    spd$intensity <- NumericList(as.numeric(x$peaks$intensities),
+                                 compress = FALSE)
+    Spectra(spd)
+}


Same here - reference is not reference explicitly.

Great catch! I will drop it here. Think this must have a copy-paste error. Or is there any reason this should be here @michaelwitting ?

I removed it - did not break any unit tests, so should be fine.

hechth · 2022-03-08T07:38:32Z

tests/testthat/test_conversion.R

+    p <- SpectriPy:::.single_rspec_to_pyspec(sps[1L])
+    res <- SpectriPy:::.single_pyspec_to_rspec(p, vars)
+    expect_equal(mz(res), mz(sps[1L]))
+    expect_equal(intensity(res), intensity(sps[1L]))
+    expect_equal(rtime(res), rtime(sps[1L]))
+    expect_equal(msLevel(res), msLevel(sps[1L]))
+
+    p <- SpectriPy:::.single_rspec_to_pyspec(sps[2L])
+    res <- SpectriPy:::.single_pyspec_to_rspec(p, vars)
+    expect_equal(mz(res), mz(sps[2L]))
+    expect_equal(intensity(res), intensity(sps[2L]))
+    expect_equal(rtime(res), rtime(sps[2L]))
+    expect_equal(msLevel(res), msLevel(sps[2L]))
+
+    p <- SpectriPy:::.single_rspec_to_pyspec(sps[3L])
+    res <- SpectriPy:::.single_pyspec_to_rspec(p, vars)
+    expect_equal(mz(res), mz(sps[3L]))
+    expect_equal(intensity(res), intensity(sps[3L]))
+    expect_equal(rtime(res), rtime(sps[3L]))
+    expect_equal(msLevel(res), msLevel(sps[3L]))


This could be solved nicely using the parameterized test cases from the patrick R package I think.

never used the patrick package thus far - I'll have a look at it.

Done. Thanks for the suggestion!!!

jorainer · 2022-03-09T12:21:33Z

I've addressed all your points @hechth - looking forward to your review.

hechth · 2022-03-10T07:16:50Z

This should also close #10, right?

jorainer · 2022-03-10T10:44:18Z

Thanks for the review @hechth ! I will also try to set up proper github action tests in the main branch.

hechth · 2022-03-10T11:47:05Z

Do you have experience with the CI for R packages? And do we want to integrate the package into conda?

jorainer · 2022-03-11T09:12:37Z

Yes, for our R-packages we use github actions that check the package on Windows, macOS and Linux. Integration into conda? I have no idea, never done that :) if you think that might be good we should do it, but how about all the dependencies then? are they already in conda?

hechth · 2022-03-11T15:22:11Z

I can check the state of the dependencies for conda.

jorainer added 6 commits March 4, 2022 15:08

feat: add basilisk to manage python and dependencies

7b7e58d

docs: update README

313b8e2

feat: add spectraVariableMapping and unit tests

3dc8172

- Modify `rspec_to_pyspec` to support `spectraVariableMapping`. - Add `spectraVariableMapping`. - Add unit tests for `rspec_to_pyspec`. - Configure with `basilisk` package.

refactor: re-write and export rspec_to_pyspec

3920d91

refactor: refactor the pyspec_to_rspec function

ff81c28

docs: update and extend vignette

00ad8fa

jorainer requested a review from michaelwitting March 8, 2022 06:57

jorainer mentioned this pull request Mar 8, 2022

Suggested first steps #1

Open

hechth reviewed Mar 8, 2022

View reviewed changes

jorainer added 2 commits March 8, 2022 16:01

refactor: remove unnecessary reference

b54f0e7

tests: add patrick parameterized tests

fe1d9ce

jorainer requested a review from hechth March 8, 2022 15:30

hechth approved these changes Mar 10, 2022

View reviewed changes

hechth mentioned this pull request Mar 10, 2022

Handle passing/initialization of reference to matchms environment #13

Open

jorainer merged commit 142fcc3 into main Mar 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Functionality to convert between R and Python #10

Functionality to convert between R and Python #10

jorainer commented Mar 8, 2022

jorainer commented Mar 8, 2022

jorainer commented Mar 8, 2022

hechth left a comment

hechth Mar 8, 2022

jorainer Mar 8, 2022

hechth Mar 8, 2022

hechth Mar 8, 2022

jorainer Mar 8, 2022

jorainer Mar 8, 2022

hechth Mar 8, 2022

hechth Mar 8, 2022

michaelwitting Mar 8, 2022

jorainer Mar 8, 2022

hechth Mar 10, 2022 •

edited

Loading

hechth Mar 8, 2022

jorainer Mar 8, 2022

michaelwitting Mar 8, 2022

michaelwitting Mar 8, 2022

hechth Mar 8, 2022

jorainer Mar 8, 2022

jorainer Mar 8, 2022

hechth Mar 8, 2022

jorainer Mar 8, 2022

jorainer Mar 8, 2022

jorainer commented Mar 9, 2022

hechth commented Mar 10, 2022

jorainer commented Mar 10, 2022

hechth commented Mar 10, 2022

jorainer commented Mar 11, 2022

hechth commented Mar 11, 2022

Functionality to convert between R and Python #10

Functionality to convert between R and Python #10

Conversation

jorainer commented Mar 8, 2022

jorainer commented Mar 8, 2022

jorainer commented Mar 8, 2022

hechth 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hechth Mar 10, 2022 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorainer commented Mar 9, 2022

hechth commented Mar 10, 2022

jorainer commented Mar 10, 2022

hechth commented Mar 10, 2022

jorainer commented Mar 11, 2022

hechth commented Mar 11, 2022

hechth Mar 10, 2022 •

edited

Loading