Skip to content

Obi preview 1 #95

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Obi preview 1 #95

wants to merge 9 commits into from

Conversation

obigriffith
Copy link
Collaborator

Purpose/implementation Section

What changes are being implemented in this Pull Request?

What was your approach?

What GitHub issue does your pull request address?

Tell potential reviewers what kind of feedback you are soliciting.

New Content Checklist

Copy link
Contributor

github-actions bot commented Apr 28, 2025

No spelling errors! 🎉
Comment updated at 2025-04-29-03:15:19 with changes from 7dcaf56

Copy link
Contributor

github-actions bot commented Apr 28, 2025

⚠️ broken url errors ⚠️
There are broken url errors that need to be addressed.
Click here ➡️ for broken url errors!
Add errors that aren't errors to the resources/ignore-urls.txt file of this repo.
If you are having troubles see this guide
Comment updated at 2025-04-29-03:15:20 with changes from 7dcaf56

Copy link
Contributor

github-actions bot commented Apr 28, 2025

Re-rendered previews from the latest commit:

* note not all html features will be properly displayed in the "quick preview" but it will give you a rough idea.

Updated at 2025-04-29 with changes from the latest commit 7dcaf56

Comment on lines +144 to +145
1. Organized these files using an organizational scheme similar to [what is described above](#example-organizational-scheme).
1. Create folders like `plots`, `results`, and `data`. Note that `aggregated_metadata.json` and `LICENSE.TXT` also belong in the `data` folder.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is a slight confusion created by the fact that the R file set does not seem to include any plots or results files and the python file set does not include any results files. So, the organization exercise doesn't follow the proposal very closely.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I feel that there is something important missing from this chapter, specifically the issue of LICENSE. The chapter title, contents and conclusion of the excellent tutorial video for creating a github repo all imply that we are starting an "open source" project. However, without adding license, the project is public but not open source. For a new github repo (even a public one) without a license, the default copyright laws apply, meaning that the author/creators retains all rights to their source code and no one may reproduce, distribute, or create derivative works. In order to be open source it must include a license which meets the definition of open source. The end of this chapter would be a good place to introduce this idea and maybe show how easy it is to add a license in github.

Comment on lines +186 to +188

<img src ="https://docs.google.com/presentation/d/1LMurysUhCjZb7DVF6KS9QmJ5NBjwWVjRn40MS9f2noE/export/png?id=1LMurysUhCjZb7DVF6KS9QmJ5NBjwWVjRn40MS9f2noE&pageid=gfaa026a583_0_13" alt="In RStudio, you can create a new notebook by going to File > New File > R Notebook. Then open up this chapter’s example code folder and open the make_heatmap.R file." style="display: block; margin: auto;" />

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note that in my browser, most of the instructions for setting up the R notebook (points 3 - 8) were hidden (under the image?) although I could see them in the source code. I am hoping they will appear when I add newlines around the code block to match formatting of python notebook section which is rendering correctly. But, this needs to be verified.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When running the renv::snapshot() command I was prompted with a dialogue:

It looks like you've called renv::snapshot() in a project that hasn't been activated yet.
How would you like to proceed?
1: Activate the project and use the project library.
2: Do not activate the project and use the current library paths.
3: Cancel and resolve the situation another way.

I wasn't sure of the correct response. It might be good to give some guidance to learners on this topic.

Comment on lines -101 to -105
`````
```{r}
sessionInfo()
```
`````
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the html of this course, this code seems to be generating not only the R-styled snippet for the sessionInfo command but also an entire example output of sessionInfo. I'm not sure but I don't think the intention is to add this sessionInfo to the Rmd file? I am guessing that the 5 back ticks are not having the intended effect here?

@@ -185,7 +185,7 @@ Try to avoid using variable names that have no meaning like `tmp` or `x`, or `i`
> 2 Use consistent notation for naming convention.
> 3 Use standard terms.
> 4 Do not number a variable name.
> 5 When you find another way to name variable, refactor as fast as possible.
> 5 When you find another way to name a variable, refactor as fast as possible.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not positive that I understand what this piece of advice is meant to convey

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I struggled with the penultimate exercise for this chapter (7.5.1). I was working off a version of the code from previous chapters (which I think should have been up to date) but it didn't quite match the "before" code I was meant to update. So, I downloaded the suggested project files (section 7.4). However, these seemed to match the "before" expectation even less well. I think they are actually the "after" solution.

@@ -15,11 +15,11 @@ ottrpal::include_slide("https://docs.google.com/presentation/d/1LMurysUhCjZb7DVF

Documentation is an important but sometimes overlooked part of creating a reproducible analysis! There are two parts of documentation we will discuss here: 1) In notebook descriptions and 2) READMEs

Both these notebook descriptions and READMEs are written in markdown -- a shorthand for html (the same as the documentation parts of your code). If you aren't familiar, markdown is such a handy tool and we encourage you to learn it (it doesn't take too long), here's a [quick guide](https://www.markdownguide.org/cheat-sheet/) to get you started.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't know that it is technically correct to call markdown a shorthand for html. It wasn't clear to me what point we are trying to make with the text in the parentheses. I tried to make this sentence more clear...

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.

1 participant