Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix use of dataclasses for python 3.11 #1047

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

simon-ball
Copy link

Suite2p is currently non-functional in Python 3.11 due to a change in the way the built-in library dataclasses handles default values: https://docs.python.org/3.11/whatsnew/3.11.html#dataclasses (cross ref #1029 )

This change modifies the default creation in two ways to address the limitation:

  • rather than directly assigning a (mutable) default, a default is generated independently in each instance by defining a default_factory instead
  • default_factory expects a function that will be called on ROI.__init__(), so move default generation to a lambda expression to be called as and when needed. Depending on style preferences, this could potentially be integrated with the suite2p.detection.distance_kernel method directly, since as far as I can tell the default in the ROI dataclass is the sole purpose of that function.

The new method is back-compatible at least as far as Python 3.8.

I have not tested for any other 3.11 incompatibilities yet

dataclass default values no longer permitted to be mutable in 3.11 (https://docs.python.org/3.11/whatsnew/3.11.html#dataclasses)

Two changes needed:
* Use a default_factory instead (ensures separate instances have independent values)
* use a lamba to generate the same default value used previously

reference MouseLand#1029
@landoskape
Copy link
Contributor

What issues in suite2p does this fix? Or does it generally not work at all with 3.11?

@simon-ball
Copy link
Author

simon-ball commented Oct 30, 2023

Suite2p is currently non-functional in Python 3.11, as in, it cannot be successfully imported. At all.

$ conda create -n test311 python=3.11 -y
$ conda activate test311
$ pip install suite2p
$ python -c "import suite2p"

Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/home/ubuntu/miniconda3/envs/imtest311/lib/python3.11/site-packages/suite2p/__init__.py", line 6, in <module>
    from .run_s2p import run_s2p, run_plane, pipeline
  File "/home/ubuntu/miniconda3/envs/imtest311/lib/python3.11/site-packages/suite2p/run_s2p.py", line 15, in <module>
    from . import extraction, io, registration, detection, classification, default_ops
  File "/home/ubuntu/miniconda3/envs/imtest311/lib/python3.11/site-packages/suite2p/extraction/__init__.py", line 5, in <module>
    from .extract import create_masks_and_extract, enhanced_mean_image, extract_traces_from_masks, extraction_wrapper
  File "/home/ubuntu/miniconda3/envs/imtest311/lib/python3.11/site-packages/suite2p/extraction/extract.py", line 11, in <module>
    from .masks import create_masks
  File "/home/ubuntu/miniconda3/envs/imtest311/lib/python3.11/site-packages/suite2p/extraction/masks.py", line 9, in <module>
    from ..detection.sparsedetect import extendROI
  File "/home/ubuntu/miniconda3/envs/imtest311/lib/python3.11/site-packages/suite2p/detection/__init__.py", line 4, in <module>
    from .detect import detect, detection_wrapper, bin_movie
  File "/home/ubuntu/miniconda3/envs/imtest311/lib/python3.11/site-packages/suite2p/detection/detect.py", line 9, in <module>
    from . import sourcery, sparsedetect, chan2detect, utils
  File "/home/ubuntu/miniconda3/envs/imtest311/lib/python3.11/site-packages/suite2p/detection/sourcery.py", line 12, in <module>
    from .stats import fitMVGaus
  File "/home/ubuntu/miniconda3/envs/imtest311/lib/python3.11/site-packages/suite2p/detection/stats.py", line 52, in <module>
    @dataclass(frozen=True)
     ^^^^^^^^^^^^^^^^^^^^^^
  File "/home/ubuntu/miniconda3/envs/imtest311/lib/python3.11/dataclasses.py", line 1220, in wrap
    return _process_class(cls, init, repr, eq, order, unsafe_hash,
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/ubuntu/miniconda3/envs/imtest311/lib/python3.11/dataclasses.py", line 958, in _process_class
    cls_fields.append(_get_field(cls, name, type, kw_only))
                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/ubuntu/miniconda3/envs/imtest311/lib/python3.11/dataclasses.py", line 815, in _get_field
    raise ValueError(f'mutable default {type(f.default)} for field '
ValueError: mutable default <class 'numpy.ndarray'> for field rsort is not allowed: use default_factory

The proximate cause of that inability to import appears to be that in 3.10 and earlier, the built-in library dataclasses allowed the default value of a class decorated with @dataclasses.dataclass to be mutable, but that in Python 3.11, this became forbidden (prior link from above: https://docs.python.org/3.11/whatsnew/3.11.html#dataclasses). As I understand it, the reason for doing so is to ensure that you cannot change the default value in all extant instances of that dataclass by overwriting it in one instance.

As the exception generated on importing explains, the object suite2p.detection.stats.ROI has a field rsort that is assigned a mutable default value, i.e. a NumPy array.

This PR attempts to address that problem as described above. In terms of the apparent goal of the change to dataclasses in 3.11, the shared mutable default value for ROI.rsort is made un-shared by instead assigning a generator/factory that will be called in ROI.__init__, resulting in identical but independent copies of the same default value.

@simon-ball
Copy link
Author

simon-ball commented Oct 30, 2023

This is, I assume the problem that the change to dataclasses in Python 3.11 is supposed to solve:

from suite2p.detection.stats import ROI
import numpy as np
z1 = np.zeros((2,2))
z2 = np.zeros((3,3))
a = ROI(z1,z1,z1,z1,False)
b = ROI(z2,z2,z2,z2,False)
a.rsort is b.rsort
>>> True
a.rsort
>>> array([ 0.        ,  1.        ,  1.        , ..., 42.42640687, 42.42640687, 42.42640687])
a.rsort[0] = -1
b.rsort
>>> array([ -1        ,  1.        ,  1.        , ..., 42.42640687, 42.42640687, 42.42640687])

By editing the (default) value of rsort in a, the value in b also changes. And that shouldn't happen.

I don't know whether it's a bug that affects Suite2p - probably not - but it was fixed in the built in library in Python 3.11, and Suite2p will have to make some change to be usable in 3.11 as a result. This change should fix it, although the extent of my testing so far is that the resulting rsort value is unchanged.

@landoskape
Copy link
Contributor

Ah I see. I think suite2p is only supported for 3.9 (that's what it says in the README, although the instructions for developers doesn't include this). It definitely seems useful to make an update that is fully back-compatible though!

@simon-ball
Copy link
Author

It definitely seems useful to make an update that is fully back-compatible though!

That was pretty much my thoughts too. I uncovered this while trying to get ahead of the impending EOL of python 3.8 (which is my current target version), and Suite2p appears to be the biggest obstruction (1) to me to choosing 3.11 as the new target. Given the work involved in re-validating everything for a new target, I'd definitely prefer to aim for 3.11 and push the next one off by another year!

(1) not intended in a negative sense - simply that it's non-optional to us right now, and it restricts us to a target no newer than 3.10.

@marius10p
Copy link
Contributor

@simon-ball thanks for finding this problem in 3.11. Bringing our dependencies forward is a big task and we don't have the manpower to do it very often. Since 3.10 EOL is in three years, we are going to hold off on this for a bit. Please feel free to maintain your own fork of suite2p with the update.

@simon-ball
Copy link
Author

Fair enough so

Bringing our dependencies forward is a big task and we don't have the manpower to do it very often

Can't disagree with that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants