Skip to content
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

Edit to categorical data section and the addition of box and whisker plots #32

Merged
merged 2 commits into from
Nov 17, 2022

Conversation

campbelle1
Copy link
Collaborator

Please let me know if this works and if you have suggestions for changes. Please don't change things without prior discussion.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@jesteria
Copy link
Member

Hello! Of course. Thanks for the pull request.

Just looking at what's committed here, I see three Notebooks added for chapter 8. I'm guessing these are/were intended for what's now chapter 9 (data visualization). Were these Notebooks added accidentally? Or are they perhaps intended to replace what's now in chapter 9? (There are of course also changes here to the files currently making up chapter 9.)

Beyond that, just looking over the other changes:

  • (Kind of trivial to mention but I also see the empty file .Rhistory added. I would think this is not needed for the repository and I can just ensure it's excluded.)

  • It's not hugely important but I see the addition of filters to ignore warnings (warnings.filterwarnings()). I would hope this isn't necessary for the textbook; though there's not great harm in it either. Of course if there's an ongoing issue relating to this I'd be happy to help with it.

Otherwise of course this looks great. I might hold off on testing it in the textbook just until I know which files are intended.

@campbelle1
Copy link
Collaborator Author

Hi Jesse,

Thanks for your reply and attention. The files for chapter 8 was a mistake on my end. Those can be deleted, as we only need the place holder file for that.

Where was the .Rhistory file added? I didn't intend to add that as well and it can be deleted.

I added a warnings command just for the Categorical data notebook. The other notebooks should be fine I believe.

1 similar comment
@campbelle1
Copy link
Collaborator Author

Hi Jesse,

Thanks for your reply and attention. The files for chapter 8 was a mistake on my end. Those can be deleted, as we only need the place holder file for that.

Where was the .Rhistory file added? I didn't intend to add that as well and it can be deleted.

I added a warnings command just for the Categorical data notebook. The other notebooks should be fine I believe.

@jesteria
Copy link
Member

OK – (as you can see above) – I've committed two changesets, one to remove the .Rhistory file and ignore these in the future, and one to remove those notebooks that were added to the chapter 8 directory.

For the moment you can review these changesets in this pull request.

And if you like you can review the end-result changes here as well.

These data visualization notebooks appear nicely compatible with the textbook and build fine. Looks good to me.

I'm curious that we need to filter warnings at all; but, that's fine, certainly there and for now.

I can incorporate this if it looks good to you as well.

@campbelle1
Copy link
Collaborator Author

OK – (as you can see above) – I've committed two changesets, one to remove the .Rhistory file and ignore these in the future, and one to remove those notebooks that were added to the chapter 8 directory.

For the moment you can review these changesets in this pull request.

And if you like you can review the end-result changes here as well.

These data visualization notebooks appear nicely compatible with the textbook and build fine. Looks good to me.

I'm curious that we need to filter warnings at all; but, that's fine, certainly there and for now.

I can incorporate this if it looks good to you as well.

Looks good to me, thanks!

@jesteria jesteria merged commit a130e21 into uchicago-dsi:master Nov 17, 2022
@jesteria
Copy link
Member

OK. This work is merged and published: https://ds1.datascience.uchicago.edu/09/data-visualization.html


FYI: Since this was pulled from your fork's main branch (master), you might just want to re-sync that branch with this repository's before beginning anything new. No need to worry about it but it might help in the future. I've outlined this for example here. (Otherwise it can help just to do work in separate branches.) Thanks.

SusannaLange pushed a commit to SusannaLange/textbook-datascience-1 that referenced this pull request Aug 16, 2023
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.

None yet

2 participants