single-cell-best-practices icon indicating copy to clipboard operation
single-cell-best-practices copied to clipboard

Suggestion to update tutorial: deprecated pandas2ri.activate() and sparse matrix conversion for SoupX

Open liberentaizp opened this issue 6 months ago • 4 comments

Describe the bug

The SoupX tutorial uses pandas2ri.activate(), which is deprecated in recent versions of rpy2, and attempts to pass scipy.sparse matrices (like adata.X.T) directly into R using rpy2, which raises a NotImplementedError.

I saw that this issue was mentioned in a closed issue [#372 ], and a solution involving manual extraction of sparse matrix components (x, i, p, dims) was provided in an unreleased version of the notebook. However, the current public version still uses the deprecated and incompatible approach.

To Reproduce

Steps to reproduce the behavior:

  1. Follow the SoupX tutorial using recent rpy2 (≥3.5).
  2. Attempt to run: ro.pandas2ri.activate() or pass a sparse matrix as %R -i data
  3. See these errors:
    • DeprecationWarning from pandas2ri.activate()
    • NotImplementedError: Conversion 'py2rpy' not defined for objects of type '<class 'scipy.sparse._csc.csc_matrix'>'

Expected behavior

It would be helpful if the tutorial:

  • Used the current rpy2-recommended method with localconverter(...), or optionally add a note about it
  • Included the unpacking-and-reconstruction approach for sparse matrices (as described in the closed issue and unreleased notebook version).

Thanks for the great work on the tutorial! 😄 I’d be happy to contribute a small PR to help update the notebook if that would be helpful!

liberentaizp avatar May 27 '25 11:05 liberentaizp

Dear @liberentaizp

thank you! What I couldn't quite get from your issue is whether the current version on main already solves all of your issues or not? If I recall correctly, it already uses localconverters. If not, we must fix this. What were you thinking of fixing with a PR?

We'll make a new release of the book in a few weeks so that the working version is also rendered.

Zethson avatar May 27 '25 14:05 Zethson

Hi!

You're right, I was just continuing the tutorial and I saw that it does already use localconverter in some places.

The main thing I ran into is in the SoupX section (part 6.4. Correction of ambient RNA), where scipy.sparse matrices like adata.X.T are passed directly to R. That still throws a NotImplementedError unless you manually unpack the matrix into x, i, p, and dims, which I saw you do in the unreleased version you linked in issue #372 , just not yet on main.

Also, on parts 6, 7 and 8 pandas2ri.activate() is still used, which raises a deprecation warning in newer rpy2 versions.

I haven't finished the full tutorial yet, so I may be missing something, but I think these updates would be worth reflecting in main.

I’d be happy to put together a small PR that replaces the remaining pandas2ri.activate() calls with the localconverter pattern, and updates the SoupX matrix handling using the unpacking/reconstruction approach from the unreleased version. Just let me know if that’d be helpful!

Thanks!! 🤸

liberentaizp avatar May 27 '25 15:05 liberentaizp

I’d be happy to put together a small PR that replaces the remaining pandas2ri.activate() calls with the localconverter pattern, and updates the SoupX matrix handling using the unpacking/reconstruction approach from the unreleased version. Just let me know if that’d be helpful!

That would be great and very helpful! I'd happily accept such a PR. Please let us know if you need any pointers, please.

Zethson avatar May 27 '25 15:05 Zethson

Quick update! I realized that I had been looking at the book version initially, and not in the main branch. 🤕 The localconverter and sparse matrix unpacking are already updated in main. However, I noticed that pandas2ri.activate() was still included in parts 6, 7, and 8, so I removed those lines in the PR since they’re now deprecated. Let me know if you'd prefer to keep any of them in for earlier rpy2 versions, but otherwise I think it should be good to go!

liberentaizp avatar May 27 '25 15:05 liberentaizp