search

AllenInstitute / openscope_databook

OpenScope databook: a collaborative, versioned, data-centric collection of foundational analyses for reproducible systems neuroscience 🐁🧠🔬🖥️📈
https://alleninstitute.github.io/openscope_databook
Other
63 stars 19 forks source link

Jiaxx review #395

Closed Laohusong closed 3 months ago

Laohusong commented 4 months ago

This is our committed branch. Take care before merging because of the version problem.

review-notebook-app[bot] commented 4 months ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

jeromelecoq commented 4 months ago

Many thanks for your contribution! We will go over this in the next weeks. Note that some team members are out of office until mid-June so we will go in bigger depth then.

rcpeene commented 3 months ago

This all looks very good! Thank you for your changes.

The only question I have is about changes pushed for the download_nwb.ipynb and use_nwbwidgets.ipynb. It looks like they both have a first cell with error output from a keyboard interrupt. Was that a mistake?

If that gets resolved I can merge this into our repo.

Laohusong commented 3 months ago

I have fixed the errors. They are just keyboard interrupts😂, sorry. (Ignore the last two commits)

rcpeene commented 3 months ago

Was there an error in your merge? It looks like a lot of the changes are actually undoing changes I have made to our main branch in the past few weeks? For instance, the diffs of build.yml, test.yml, contributors.csv, etc.

It could be pretty harmful to accidentally undo a lot of these changes, perhaps you need to try to redo the merge or pull our main into your local branch?

Laohusong commented 3 months ago

I am sorry for the confusing merges and undos. The github automatically merged the branch before pushing. And I reverted it. So the last two commits made no changes in total.

Laohusong commented 3 months ago

perhaps you need to try to redo the merge or pull our main into your local branch?

But I agree with it. The commits are based on an older version, so it may be necessary to pull the main and test again. I will do it later.

rcpeene commented 3 months ago

Alright thanks!

Laohusong commented 3 months ago

I tested the commits in a new PR, which is based on the new main branch. This one will be closed.