search

matthewfeickert / R-in-Jupyter-with-Binder

Example of how to use R in Jupyter notebooks and make compatible with Binder
BSD 3-Clause "New" or "Revised" License
17 stars 6 forks source link

Get feedback from Achintya on clarity #5

Closed matthewfeickert closed 6 years ago

matthewfeickert commented 6 years ago

@RaoOfPhysics, can I get you to look over this repo and give feedback?

RaoOfPhysics commented 6 years ago

@matthewfeickert, this is stellar stuff. Seriously, it's beautiful. :D

General comments

Comments on the notebook

Comments on the README

Overall, looks good for now! We can revise and refine later if needed. :)

matthewfeickert commented 6 years ago

@RaoOfPhysics Thanks so much for the feedback! I really value it, and it is great!

I'll revise this over the next few evenings and then see if you have any further feedback. :)

matthewfeickert commented 6 years ago

General comments

  • You can (and might want?) your launch binder button to link to a notebook file directly, if you have only one file (of interest) in the repo

Yes, we do this for pyhf. I generally like the idea of not launching into the Notebook right away as it shows the user everything else going on, or what else might be of interest, but I think that since this is a "getting going" example repo and that there is only 1 notebook that it is a better idea to launch into it. Thanks for pointing this out!

  • Thank you for re-starting the kernel and running all cells before pushing the notebook to GitHub. :D It pleases me that all the cell numbers are consecutive and don't jump from, say, 3 to 41. And thank you for including this in your recommendations in the notebook itself.

Absolutely! :) I'm a big proponent of "Restart and Run All or it didn't happen", so I'm very happy to hear that you are also in this camp. 👍

Comments on the notebook

  • [x] [Minor] "non base" → "non-base"
  • When you say you can call non-base packages, mention that these need to be installed first, either on one's local system if using Jupyter locally or via the binder instructions for use on the web.

I've added some extra text on this now (hopefully not too dry and not too much).

  • Not critical but you might want to explain what exactly papermillR does in that cell.

I've tried to go into this a bit without going in too deep. Let me know if you think I should expand.

Comments on the README

  • For someone like me who has no idea how papermill works, it would perhaps help to include a "pipeline" visualisation to show what exactly happens with it? If you do this, you might want to it for all sections. It was only because of my bias that I requested this, as all the other sections were familiar to me and therefore obvious.

I agree that the papermill section could use a bit more detail, but I think if I'm going to put in the work to do so I should just open a PR on the papermill docs instead (great project, but the docs could use some work). Maybe I can follow up on this (and all other sections) in a later revision of this project?

  • Provide alternatives to GitHub's CI offerings? GitLab has free GitLab CI for all repos (public and private), and can be self-hosted.

I additionally mention CircleCI, but can you expand on this a bit? Are you suggesting adding a link to GitLab's CI service for repos that are hosted on GitHub?

Awesome link! Yes!

RaoOfPhysics commented 6 years ago

I generally like the idea of not launching into the Notebook right away as it shows the user everything else going on, or what else might be of interest, but I think that since this is a "getting going" example repo and that there is only 1 notebook that it is a better idea to launch into it. Thanks for pointing this out!

You have a good point, and I think since this is meant to serve as an example for using Binder, it would make sense to point to the root directory rather than the specific notebook. You could always add a sentence saying, "BTW, you can also link to a specific notebook directly if you prefer, like [THIS]."

I've tried to go into this [papermillR] a bit without going in too deep. Let me know if you think I should expand.

This helps! Thanks for making it clear that it's not really useful within the notebook environment itself.

I agree that the papermill section could use a bit more detail, but I think if I'm going to put in the work to do so I should just open a PR on the papermill docs instead (great project, but the docs could use some work). Maybe I can follow up on this (and all other sections) in a later revision of this project?

Makes sense. :)

Provide alternatives to GitHub's CI offerings? GitLab has free GitLab CI for all repos (public and private), and can be self-hosted. I additionally mention CircleCI, but can you expand on this a bit? Are you suggesting adding a link to GitLab's CI service for repos that are hosted on GitHub?

I meant that you could point out that people can self-host GitLab (at their universities?) and use the baked-in CI tools for reprodubility, and can use this privately if needed. I think openness and transparency are key, but sometimes you need to stress-test things internally. :)

RaoOfPhysics commented 6 years ago

Also, CHECK THIS OUT!

https://github.com/mwouts/nbrmd/issues/19