Skip to content

Conversation

oscarbranson
Copy link
Contributor

Mangled the last one (#89) so badly that I created a new PR... hope this is better @aloctavodia!

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

Review Jupyter notebook visual diffs & provide feedback on notebooks.


Powered by ReviewNB

@review-notebook-app
Copy link

View / edit / reply to this conversation on ReviewNB

aloctavodia commented on 2020-06-12T21:04:54Z
----------------------------------------------------------------

nitpicking: no need for the legend "Statistical Rethinking, 2nd Edition", right?


@review-notebook-app
Copy link

View / edit / reply to this conversation on ReviewNB

aloctavodia commented on 2020-06-12T21:04:54Z
----------------------------------------------------------------

If you use az.plot_kde you will be able to remove "ax.set_xlim(0, 36)".


@review-notebook-app
Copy link

View / edit / reply to this conversation on ReviewNB

aloctavodia commented on 2020-06-12T21:04:55Z
----------------------------------------------------------------

fix typo --> "Evaluate potential and kinetic energies at start and end of trajectory"


@review-notebook-app
Copy link

View / edit / reply to this conversation on ReviewNB

aloctavodia commented on 2020-06-12T21:04:56Z
----------------------------------------------------------------

We may ask Richard for guidance.


@review-notebook-app
Copy link

View / edit / reply to this conversation on ReviewNB

aloctavodia commented on 2020-06-12T21:04:56Z
----------------------------------------------------------------

This is already supported, right? (or only on master?)


@review-notebook-app
Copy link

View / edit / reply to this conversation on ReviewNB

aloctavodia commented on 2020-06-12T21:04:57Z
----------------------------------------------------------------

maybe set alpha to better mimic the plot in the book?


@review-notebook-app
Copy link

View / edit / reply to this conversation on ReviewNB

aloctavodia commented on 2020-06-12T21:04:58Z
----------------------------------------------------------------

This is already possible


@AlexAndorra
Copy link
Contributor

Hi @oscarbranson !
Did you have any questions about @aloctavodia's review or is everything clear?

@oscarbranson
Copy link
Contributor Author

Thanks @aloctavodia and @AlexAndorra - I haven't had time to look at it yet... I hope to get to it this weekend.

@AlexAndorra
Copy link
Contributor

Hi @oscarbranson ! Do you think you'll have some time to move this across the finish line? 🏁

@oscarbranson
Copy link
Contributor Author

Yes! Apologies for the delay, @AlexAndorra... I've been away. Should get to it early this week.

@AlexAndorra
Copy link
Contributor

AlexAndorra commented Aug 6, 2020

Let's move this one along: merging now so that people can already use it, and when someone has time he can implement the changes @aloctavodia mentioned above 😉

@AlexAndorra AlexAndorra merged commit b7d4694 into pymc-devs:master Aug 6, 2020
@oscarbranson
Copy link
Contributor Author

Apologies @AlexAndorra and @aloctavodia - it's been a hectic couple of months. I'll try to revisit this soon, but probably a good idea to merge for now - it mostly works as-is!

@aloctavodia
Copy link
Member

Do not worry @oscarbranson we are living crazy times. And you already helped a lot! Thanks for your contributions.

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