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

JNB:BUG:ENH: Fix Suite2p example, and update Binder environment for Python 3.9 #220

Closed
wants to merge 7 commits into from

Conversation

swkeemink
Copy link
Member

With this PR (which replaces an earlier PR which I accidentally botched a bit) I:

  • Update the Suite2p notebook to work with the latest Suite2p API changes
  • Update the Suite2p notebook to fix the bug in which multiprocessing freezes (fixing Stuck on 'Doing signal separation...' #147)
  • Update the binder environment to work with the current Suite2p in Python 3.9
  • Removed the link to the Sima notebook in binder, and clarified that it does not currently work in Python 3.9

@swkeemink swkeemink requested a review from scottclowe June 29, 2021 16:25
@codecov
Copy link

codecov bot commented Jun 29, 2021

Codecov Report

Merging #220 (7ae8584) into master (ba0680e) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #220   +/-   ##
=======================================
  Coverage   93.05%   93.05%           
=======================================
  Files           8        8           
  Lines         922      922           
  Branches      196      196           
=======================================
  Hits          858      858           
  Misses         33       33           
  Partials       31       31           
Flag Coverage Δ
nbsmoke 65.29% <ø> (ø)
unittests 92.73% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ba0680e...7ae8584. Read the comment docs.

@swkeemink
Copy link
Member Author

On Binder there are currently two bugs.

Matplotlib is missing, which of course breaks Holoviews.

Suite2p does not import and throws the following error (from pyqt5):
ImportError: libGL.so.1: cannot open shared object file: No such file or directory

Copy link
Member

@scottclowe scottclowe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some commits (4de477d and 9d5e34b) are undoing formatting style fixes I made to the suite2p and SIMA notebooks. I would appreciate if these commits were dropped from the PR. I expect this has come from checking out the new branch, using your notebooks that were forked from the previous (old) copies of these notebooks, overwriting the notebooks, and then committing everything in the notebook without checking what it is that you're committing. If you're actually including these changes on purpose, please let me know.

@@ -40,7 +43,7 @@
"import holoviews as hv\n",
"\n",
"%load_ext holoviews.ipython\n",
"%output widgets=\"embed\""
"%output widgets='embed'"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unwanted side-effect.

Comment on lines 232 to 233
" i_cell: hv.Curve(exp.raw[i_cell][i_trial][0, :], label=\"SIMA\")\n",
" * hv.Curve(exp.result[i_cell][i_trial][0, :], label=\"FISSA\")\n",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unwanted side-effect.

Comment on lines 215 to 216
" i_cell: hv.Curve(exp.raw[i_cell][i_trial][0, :], label=\"suite2p\")\n",
" * hv.Curve(exp.result[i_cell][i_trial][0, :], label=\"FISSA\")\n",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unwanted side-effect.

Comment on lines 241 to 242
"avg_img * hv.HoloMap(region_plots, kdims=[\"Cell\"]) * fig + hv.HoloMap(\n",
" traces_plots, kdims=[\"Cell\"]\n",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unwanted side-effect.

Comment on lines 232 to 233
"avg_img * cell_locs * hv.HoloMap(region_plots, kdims=[\"Cell\"]) + hv.HoloMap(\n",
" traces_plots, kdims=[\"Cell\"]\n",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unwanted side-effect.

README.rst Outdated Show resolved Hide resolved
- scipy==1.7.0
- pip=21.1.2
- six==1.16.0
- holoviews==1.13.2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe current issues with holoviews on Binder come from it being conda installed instead of pip installed somehow?

.binder/environment.yml Outdated Show resolved Hide resolved
Comment on lines +6 to +11
- python=3.9.5
- numpy==1.21.0
- scipy==1.7.0
- pip=21.1.2
- six==1.16.0
- holoviews==1.13.2
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alphabetise

Suggested change
- python=3.9.5
- numpy==1.21.0
- scipy==1.7.0
- pip=21.1.2
- six==1.16.0
- holoviews==1.13.2
- holoviews==1.13.2
- numpy==1.21.0
- pip=21.1.2
- python=3.9.5
- scipy==1.7.0
- six==1.16.0

@scottclowe
Copy link
Member

Maybe it would be better if we move the Suite2p example and its binder to a separate repo? Then the binder would boot up quickly for the non-Suite2p examples and be more stable. What do you think?

@swkeemink
Copy link
Member Author

Not a terrible idea, although I don't like the repository bloat that comes with that, but given that Suite2p is so widely used it could be ok. We could also sway we only use Binder for our core notebooks and not the external packages (like suite2p and Sima).
With could add a note that we don't provide support for Suite2p, but only for the interaction with FISSA or something.

If we do a separate repository for Suite2p: that would just be an empty repository with only the notebook for suite2p and the binder stuff? Would you be ok to set up the repository with Binder support, and I'll add and debug the suite2p stuff?

@swkeemink
Copy link
Member Author

Just to confirm that #227 has fixed the multiprocessing bugs with suite2p and fissa together. So the changes to the notebook for multiprocessing are no longer needed.

@scottclowe
Copy link
Member

Just to confirm that #227 has fixed the multiprocessing bugs with suite2p and fissa together. So the changes to the notebook for multiprocessing are no longer needed.

Great to hear! In that case, commit b852708 can be dropped from the PR.

If #228 is merged in, that might fix the issues with the matplotlib backend to holoviews (by not using holoviews any more and just matplotlib directly).

@scottclowe
Copy link
Member

Let's drop suite2p from this binder and stay with Python 3.6 to support SIMA. Then we can make a new repository named fissa-example-suite2p for suite2p.

@swkeemink
Copy link
Member Author

Closing this PR since most of it is no longer relevant.

@swkeemink swkeemink closed this Jul 19, 2021
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.

2 participants