rochefort-lab / fissa

A Python toolbox for Fast Image Signal Separation Analysis, designed for Calcium Imaging data.
GNU General Public License v3.0
30 stars 28 forks source link

JNB:DOC:RF: update Binder environment and move Suite2p example to different Binder #259

Closed swkeemink closed 3 years ago

swkeemink commented 3 years ago

With this update we add a separate Binder environment for Suite2p to avoid version incompatibilities with SIMA. Additionally we remove all the Suite2p dependencies from the standard Binder.

We also update the Binder environment to no longer have Holoviews dependencies, and use Matplotlib instead.

See here for the Suite2p example repository, and here for the binder.

codecov[bot] commented 3 years ago

Codecov Report

Merging #259 (789246d) into master (40e1a40) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #259   +/-   ##
=======================================
  Coverage   94.12%   94.12%           
=======================================
  Files           8        8           
  Lines        1209     1209           
  Branches      302      302           
=======================================
  Hits         1138     1138           
  Misses         36       36           
  Partials       35       35           
Flag Coverage Δ
nbsmoke 59.88% <ø> (ø)
unittests 93.79% <ø> (ø)

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 40e1a40...789246d. Read the comment docs.

scottclowe commented 3 years ago

I think this PR should instead remove the suite2p notebook from the fissa repo entirely and change the README to link to the assets now in https://github.com/rochefort-lab/fissa-suite2p-example. It would be better not to have the notebook duplicated - we don't want to have to update it in two places.

swkeemink commented 3 years ago

I think this PR should instead remove the suite2p notebook from the fissa repo entirely and change the README to link to the assets now in https://github.com/rochefort-lab/fissa-suite2p-example. It would be better not to have the notebook duplicated - we don't want to have to update it in two places.

Good point. Updated that now.

Should we also remove all mentions from .appveyor.yml, and perhaps somewhere else I'm forgetting in the testing backend?

scottclowe commented 3 years ago

Should we also remove all mentions from .appveyor.yml,

No, we still run tests on AppVeyor. It is still free and we can run one Windows env on AppVeyor while we run another on GitHub.

and perhaps somewhere else I'm forgetting in the testing backend?

I'm not sure what you are forgetting, so I don't know!

swkeemink commented 3 years ago

Should we also remove all mentions from .appveyor.yml,

No, we still run tests on AppVeyor. It is still free and we can run one Windows env on AppVeyor while we run another on GitHub.

and perhaps somewhere else I'm forgetting in the testing backend?

I'm not sure what you are forgetting, so I don't know!

I meant removing the suite2p parts of it =)

And the tests seem to have failed which I assume is because of this!

scottclowe commented 3 years ago

I meant removing the suite2p parts of it =)

And the tests seem to have failed which I assume is because of this!

Yes, we need to remove references to suite2p in the GitHub tests and AppVeyor tests. They are trying to remove the Suite2p notebook so they don't test it when testing the other notebooks, and it breaks when the notebook does not exist.

Please refactor the PR so you are just removing the Suite 2p HTML and not modifying it in one commit and then removing it in another (it is a large asset to add to the history).

Before this PR goes through, I will add a HTML redirect for the Suite 2p HTML to the new path in https://github.com/rochefort-lab/fissa-suite2p-example.

swkeemink commented 3 years ago

Refactored and updated the testing to not have Suite2p mentions (hopefully)