MouseLand / suite2p

cell detection in calcium imaging recordings
http://www.suite2p.org
GNU General Public License v3.0
334 stars 239 forks source link

BUG: Toggling ROIs off and on with spacebar causes a QGraphicsScene issue #1028

Closed landoskape closed 8 months ago

landoskape commented 10 months ago

Describe the issue:

Toggling ROIs off and on does not work due to a bug in evaluating the state of the ROIs_on checkbox.

Reproduce the code example:

suite2p
# (drag stat.npy file to suite2p)
# (press spacebar)
# (press spacebar)

The code that generates the warning (and failure to reload the ROIs) is here:

def ROIs_on(self, state):
        if state == QtCore.Qt.Checked:
            self.ops_plot["ROIs_on"] = True
            self.p1.addItem(self.color1)
            self.p2.addItem(self.color2)
        else:
            self.ops_plot["ROIs_on"] = False
            self.p1.removeItem(self.color1)
            self.p2.removeItem(self.color2)
        self.win.show()
        self.show()

state is an integer, with 0 corresponding to unchecked and 2 corresponding to checked. However, QtCore.Qt.Checked is an enum, so the if statement always evalutates to False.

This issue is fixed by converting state to an enum as follows: QtCore.Qt.CheckState(state). I opened pull request #1029 to resolve it.

Warning message:

QGraphicsScene::removeItem: item 0x2dfc00f4060's scene (0x0) is different from this scene (0x2dfc015d070)
QGraphicsScene::removeItem: item 0x2dfc00fa900's scene (0x0) is different from this scene (0x2dfc015d070)

(An error isn't generated, but the code fails to replot the ROIs).

Version information:

v0.14.2

Context for the issue:

This issue prevents the user from being able to look at the images behind the ROIs while doing QC!

landoskape commented 10 months ago

FYI - the same bug will cause an issue with the "roi_text" checkbox and the "zoom to ROI" checkbox.

Andrianarivelo commented 8 months ago

exact same issue here !

landoskape commented 8 months ago

@Andrianarivelo good to know it's just not an idiosyncrasy of my setup... but for the record if you want to get it to work before it's fixed, my PR (#1029) solves it! Only a few lines in one file need to be changed.

Andrianarivelo commented 8 months ago

Which lines exactly do I have to change ?

landoskape commented 8 months ago

This page shows the lines. Alternatively you could clone from my fork which is up to date with all the edits in the master/main branch.

Then install using the developers instructions in the README file. Note that you have to make sure to specify you're in a python 3.9 environment, so change the following line in the instructions:

# don't use this one:
# conda env create --name suite2p 

# use this instead:
conda env create --name suite2p python=3.9
Andrianarivelo commented 8 months ago

Thanks, the issue is fixed for me :)

landoskape commented 8 months ago

Woot!

chriski777 commented 8 months ago

Hi all, glad to hear that @landoskape's solution worked for you, @Andrianarivelo! Do y'all by chance both have Windows machines? We're having trouble replicating this on a Mac & Linux machine.

Andrianarivelo commented 8 months ago

Yes I'm on windows, I also use Linux and don't have this issue.

landoskape commented 8 months ago

I am on windows also

landoskape commented 8 months ago

@chriski777 does my suggested fix not work on linux or mac machines? Happy to test something else on Windows to make sure the suite2p code works on every OS

chriski777 commented 8 months ago

Thanks for your responses, everyone! @landoskape, ah no, it's not that. The problem was just not reproducible on a linux/Mac machine so just wanted to confirm it's machine-specific before validating your fix!

chriski777 commented 8 months ago

1029 has been merged so now closing this issue. Thanks again, @landoskape, for the fix!