Skip to content

Conversation

@eriktks
Copy link
Member

@eriktks eriktks commented Mar 29, 2021

Ref: #7

Refactored notebook test_five_models.ipynb:

  • split main code block in smaller blocks
  • added more comments

To test: run notebook and verify that everything works

Note: I tried to add reviewnb.com's notebook reviewer but could not do that for this repository because it was not in my personal organization

Copy link
Member

@sverhoeven sverhoeven left a comment

Choose a reason for hiding this comment

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

I like that you put the sub plots in a single column that makes it easier to see how they combine.

Reviewed on https://app.reviewnb.com/sequgen/notebooks/pull/13/

Why was printing of version of sequgen removed?

@eriktks
Copy link
Member Author

eriktks commented Mar 30, 2021

Printing the sequgen version was removed because I did not see the benefit of having it, from a user point of view. Probably I am missing something here.

Thanks for adding this repository to Reviewnb. However, I do not see a review there, only Notebooks, Commits and Pull Requests.

Copy link
Member

@sverhoeven sverhoeven left a comment

Choose a reason for hiding this comment

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

OK, pinning sequgen version in

git+https://github.com/sequgen/sequgen
is propably easier.

I only used reviewnb to look at the diff as I could not comment on the deleted cell.
I think when a new PR created then Reviewnb adds a comment with a link.

@eriktks eriktks merged commit 58bfb06 into main Mar 30, 2021
@eriktks eriktks deleted the 07-refactor-test_five_models.ipynb branch March 30, 2021 11:03
@eriktks
Copy link
Member Author

eriktks commented Mar 30, 2021

Ok, thanks!

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