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

Matplotlib #230

Merged
merged 20 commits into from
Feb 23, 2022
Merged

Matplotlib #230

merged 20 commits into from
Feb 23, 2022

Conversation

jukent
Copy link
Contributor

@jukent jukent commented Feb 18, 2022

Attempt to resolve #170 from @mgrover1

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@github-actions
Copy link

github-actions bot commented Feb 18, 2022

This pull request is being automatically built with GitHub Actions and Netlify. To see the status of your deployment, click below.

🔍 Git commit SHA: 33cf10c
✅ Deployment Preview URL: https://62157134deccef400700e29b--pythia-foundations.netlify.app

@jukent
Copy link
Contributor Author

jukent commented Feb 18, 2022

I've addressed @michaelavs and @brian-rose 's comments to #170 where @dopplershift was also a reviewer

@jukent jukent marked this pull request as ready for review February 19, 2022 00:44
@jukent jukent requested a review from a team as a code owner February 19, 2022 00:44
@jukent jukent requested review from dopplershift, anissa111, mgrover1, brian-rose and michaelavs and removed request for a team, dopplershift and anissa111 February 19, 2022 00:44
@@ -0,0 +1,744 @@
{
Copy link
Contributor

@mgrover1 mgrover1 Feb 21, 2022

Choose a reason for hiding this comment

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

The verbs here should be consistent - consider changing "Learn" in the first two bullets to "Learning"


Reply via ReviewNB

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed all these bullets to match the section headings.

core/matplotlib/additional-topics2.ipynb Show resolved Hide resolved
Copy link
Contributor

@mgrover1 mgrover1 left a comment

Choose a reason for hiding this comment

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

Looks great - thanks for making those changes! Just a couple of minor comments, also for some reason the Pandas notebook was changed too, with the small change being that your branch uses master instead main for the branch... not sure why that change is tracked here...

Copy link
Member

@brian-rose brian-rose left a comment

Choose a reason for hiding this comment

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

I resolved several comment threads, and opened a few new ones over at ReviewNB. I'm not sure why my comments are showing up here on GitHub. I had the same problem in #170. But my comments should be visible on ReviewNB.

@jukent jukent mentioned this pull request Feb 22, 2022
Copy link
Contributor

@mgrover1 mgrover1 left a comment

Choose a reason for hiding this comment

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

Looks great @jukent - thanks for making those changes!

@brian-rose
Copy link
Member

@jukent there seem to be new changes in this PR and also in #170 that have diverged from each other. E.g. I can't see the dropdown menus for the colormaps in this version. Can you attempt to get all the changed merged into this PR?

@jukent
Copy link
Contributor Author

jukent commented Feb 22, 2022

Looks great - thanks for making those changes! Just a couple of minor comments, also for some reason the Pandas notebook was changed too, with the small change being that your branch uses master instead main for the branch... not sure why that change is tracked here...

This PR kept failing because of a link in the pandas notebook being broken. I should have made that change in a separate PR, but I just quickly did it here.

@jukent
Copy link
Contributor Author

jukent commented Feb 22, 2022

@brian-rose Thanks for catching that, it should be updated now.

Copy link
Member

@brian-rose brian-rose left a comment

Choose a reason for hiding this comment

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

I tried to fix one link and noted a small formatting issue. Everything else looks great!

core/matplotlib/additional-topics2.ipynb Outdated Show resolved Hide resolved
core/matplotlib/additional-topics2.ipynb Outdated Show resolved Hide resolved
core/matplotlib/additional-topics2.ipynb Outdated Show resolved Hide resolved
core/matplotlib/additional-topics2.ipynb Outdated Show resolved Hide resolved
core/matplotlib/additional-topics2.ipynb Outdated Show resolved Hide resolved
core/matplotlib/additional-topics2.ipynb Show resolved Hide resolved
jukent and others added 8 commits February 22, 2022 15:09
@jukent jukent requested a review from brian-rose February 22, 2022 22:26
@jukent jukent merged commit f5ee786 into main Feb 23, 2022
@jukent jukent deleted the matplotlib branch February 23, 2022 00:04
@jukent jukent added the content Content related issue label Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
content Content related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants