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

DOCS Updating the Rugby docs #2766

Merged
merged 1 commit into from Jan 6, 2018

Conversation

Projects
None yet
4 participants
@springcoil
Member

springcoil commented Dec 29, 2017

I've updated the Hierarchical Rugby model in the Documentation.

I've added - more data from more years, since before it only had 2014.

I've updated a few things in terms of the explanations, added a few more visualizations/ exploratory data analysis and added a few links to other resources.

I'd appreciate some proof reading, and for someone to verify if they run correctly.

@springcoil

This comment has been minimized.

Show comment
Hide comment
@springcoil

springcoil Dec 29, 2017

Member

Anyone know why this build is breaking? @twiecki @junpenglao ?

Member

springcoil commented Dec 29, 2017

Anyone know why this build is breaking? @twiecki @junpenglao ?

@ColCarroll

This comment has been minimized.

Show comment
Hide comment
@ColCarroll

ColCarroll Dec 29, 2017

Member

It looks like travis randomly killed everything... I restarted all builds

Member

ColCarroll commented Dec 29, 2017

It looks like travis randomly killed everything... I restarted all builds

@ColCarroll

This comment has been minimized.

Show comment
Hide comment
@ColCarroll

ColCarroll Dec 29, 2017

Member

Oh, you might need to rebase off master: we pinned Theano to 0.9.0 for some reasons (#2728), but it looks like it is trying to install Theano 1.0.1 right now.

Notebook diffs are hard to see what changed! Generally there are a few edits I would make on the introduction. Feel free to ignore (especially ones that aren't associated with this change), and I can make a bunch of nits myself in a future PR.

  • First sentence should be something like

I came across the following blog post on Daniel Weitzenfeld's blog, based on the work of Baio and Blangiardo.

Note that I changed the link for the Baio and Blangiardo paper, since the old one is dead.

  • Remove the bullet points in the introduction, since most of them are not referred to later. The first (about which countries are in the Six Nations) is useful to know, but should be included in the previous sentence:

Since I am a rugby fan I decide to apply the results of the paper to the Six Nations Championship, which is a competition between Italy, Ireland, Scotland, England, France and Wales.

  • The last sentence of the introduction should be updated for the new data

  • The "Motivation" section might also be reworked. There is some capitalization which should not be there, and a stray close parens. I would drop the TODO as well.

  • You might reorder the data_csv_201X section to be chronological. You could also add a "year" column, and dump the csv in "pymc3/examples/data/rugby.csv", then use df_all = pd.read_csv(pm.get_data('rugby.csv'))

Member

ColCarroll commented Dec 29, 2017

Oh, you might need to rebase off master: we pinned Theano to 0.9.0 for some reasons (#2728), but it looks like it is trying to install Theano 1.0.1 right now.

Notebook diffs are hard to see what changed! Generally there are a few edits I would make on the introduction. Feel free to ignore (especially ones that aren't associated with this change), and I can make a bunch of nits myself in a future PR.

  • First sentence should be something like

I came across the following blog post on Daniel Weitzenfeld's blog, based on the work of Baio and Blangiardo.

Note that I changed the link for the Baio and Blangiardo paper, since the old one is dead.

  • Remove the bullet points in the introduction, since most of them are not referred to later. The first (about which countries are in the Six Nations) is useful to know, but should be included in the previous sentence:

Since I am a rugby fan I decide to apply the results of the paper to the Six Nations Championship, which is a competition between Italy, Ireland, Scotland, England, France and Wales.

  • The last sentence of the introduction should be updated for the new data

  • The "Motivation" section might also be reworked. There is some capitalization which should not be there, and a stray close parens. I would drop the TODO as well.

  • You might reorder the data_csv_201X section to be chronological. You could also add a "year" column, and dump the csv in "pymc3/examples/data/rugby.csv", then use df_all = pd.read_csv(pm.get_data('rugby.csv'))

@springcoil

This comment has been minimized.

Show comment
Hide comment
@springcoil

springcoil Dec 29, 2017

Member

I made those changes you suggested Colin.

Member

springcoil commented Dec 29, 2017

I made those changes you suggested Colin.

@springcoil

This comment has been minimized.

Show comment
Hide comment
@springcoil

springcoil Dec 29, 2017

Member

If this builds correctly we can merge it I think. There's a few nitpicks etc, but I could fix them or @ColCarroll at some other point.

Member

springcoil commented Dec 29, 2017

If this builds correctly we can merge it I think. There's a few nitpicks etc, but I could fix them or @ColCarroll at some other point.

DOCS Updating the Rugby docs
Making changes based on what Colin suggested

Forgot the rugby.csv file
@ColCarroll

This comment has been minimized.

Show comment
Hide comment
@ColCarroll

ColCarroll Dec 29, 2017

Member

This is really good - I like that we have a new data set! Feel free to merge whenever - you did not write any code that is run in the test suite, so travis is not going to say anything useful. (I am not merging right now because I am interested to see if it passes, or if travis is still unhappy for other reasons)

Member

ColCarroll commented Dec 29, 2017

This is really good - I like that we have a new data set! Feel free to merge whenever - you did not write any code that is run in the test suite, so travis is not going to say anything useful. (I am not merging right now because I am interested to see if it passes, or if travis is still unhappy for other reasons)

@springcoil

This comment has been minimized.

Show comment
Hide comment
@springcoil

springcoil Dec 29, 2017

Member
Member

springcoil commented Dec 29, 2017

@springcoil

This comment has been minimized.

Show comment
Hide comment
@springcoil

springcoil Dec 29, 2017

Member
Member

springcoil commented Dec 29, 2017

@AustinRochford

This comment has been minimized.

Show comment
Hide comment
@AustinRochford

AustinRochford Dec 31, 2017

Member

If you're using format instead of f-strings, you can take the lambda out of this expression

CONVERGENCE_TITLE = lambda: "BFMI = {}\nGelman-Rubin = {}".format(bfmi, max_gr)
Member

AustinRochford commented Dec 31, 2017

If you're using format instead of f-strings, you can take the lambda out of this expression

CONVERGENCE_TITLE = lambda: "BFMI = {}\nGelman-Rubin = {}".format(bfmi, max_gr)
@AustinRochford

This comment has been minimized.

Show comment
Hide comment
@AustinRochford

AustinRochford Jan 1, 2018

Member

Looks good overall! My only comment is that you may be able to avoid duplicating some of the model math in simulate_season with sample_ppc.

Member

AustinRochford commented Jan 1, 2018

Looks good overall! My only comment is that you may be able to avoid duplicating some of the model math in simulate_season with sample_ppc.

@springcoil

This comment has been minimized.

Show comment
Hide comment
@springcoil

springcoil Jan 1, 2018

Member
Member

springcoil commented Jan 1, 2018

@junpenglao junpenglao merged commit 586024d into master Jan 6, 2018

1 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build failed
Details
coverage/coveralls Coverage increased (+0.03%) to 86.033%
Details
@junpenglao

This comment has been minimized.

Show comment
Hide comment
@junpenglao
Member

junpenglao commented Jan 6, 2018

Thanks @springcoil!

@junpenglao junpenglao deleted the DOCS-Rugby-Update branch Jan 6, 2018

@springcoil

This comment has been minimized.

Show comment
Hide comment
@springcoil

springcoil Jan 6, 2018

Member

Anyone know why the build is still failing?

Member

springcoil commented Jan 6, 2018

Anyone know why the build is still failing?

@junpenglao

This comment has been minimized.

Show comment
Hide comment
@junpenglao

junpenglao Jan 6, 2018

Member

Travis CI is not building correctly in py2.7 on master for a while now. @twiecki will contact the Travis support.

Member

junpenglao commented Jan 6, 2018

Travis CI is not building correctly in py2.7 on master for a while now. @twiecki will contact the Travis support.

jordan-melendez added a commit to jordan-melendez/pymc3 that referenced this pull request Feb 6, 2018

DOCS Updating the Rugby docs (pymc-devs#2766)
Making changes based on what Colin suggested

Forgot the rugby.csv file
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment