Adding Item Response Theory tutorial #1040
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
|
@jpchen has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
View / edit / reply to this conversation on ReviewNB wtaha commented on 2021-10-07T19:33:34Z Excited to see this posted! This is the first time for me to use revewNB, and I am in particular unsure how I will get notifications on followups to comments etc. So, if you do not here from me, please direct message or email me. Thank you! ndmlny-qs commented on 2021-10-08T17:05:52Z You should get an email just like commenting in GH. wtaha commented on 2021-10-08T17:42:16Z I have started getting these emails now, so, I think we are all set! |
|
View / edit / reply to this conversation on ReviewNB wtaha commented on 2021-10-07T19:33:35Z This is a great opening! One thing to note is that existing tutorials single our a "problem" that motivates the exposition (see existing tutorials for examples). From the above it seems that the problem is something like "predicting when NBA players will receive a foul call from a referee". It would be great to have both an opening sentence such as this one and in addition a problem section similar to the other tutorials. The opening sentence in this case can focus on IRT and also mention the reference and the sources of data. wtaha commented on 2021-10-08T20:56:13Z P.S. dhavide, FYI, the "Problem" section was part of the template we had for initial set of tutorials. |
|
View / edit / reply to this conversation on ReviewNB wtaha commented on 2021-10-07T19:33:35Z Thank you for thinking explicitly about the learning outcomes! This is really great! This is super awesome for us to have at the meta-level to check the quality of our overall curriculum delivered by the tutorials. Within the tutorial, however, this can slow the reader down a bit. Maybe learning outcomes can be collected on a separate page that points the different tutorials delivering the content that realizes these learning outcomes? dhavide commented on 2021-10-08T18:42:19Z @wtaha, I believe the significance of your concern depends on the skill level of the intended audience. For novices, explicit learning outcomes are really helpful (actually, I'd say they're useful to experts as well; they tell me whether I can skip a whole tutorial outright). IMHO, the benefits of including explicit learning outcomes outweigh this concern of making the text slightly longer. I think including learning outcomes in this way doesn't slow down a user meaningfully—skimming or skipping a bulleted list is not a big deal. If the concern is that the notebooks are too long, we can edit the prose judiciously.
So, given the points above (and that including learning outcomes within tutorials is consistent with the template we've been applying to all the tutorials), I suggest we leave these outcomes as they are. wtaha commented on 2021-10-08T20:54:46Z Hello @dhavide, thank you for sharing your perspective on this section. It's also great that you have brought up the template. We also had one when we started working on the tutorials a year ago, and it will definitely be good to have one for the set of tutorials that we end up with. I am happy for you drive that process. |
|
View / edit / reply to this conversation on ReviewNB wtaha commented on 2021-10-07T19:33:36Z Excellent list! Would you consider omitting beanmachine (and torch) given the context (the beanmachine tutorial, and beanmachine being based on pytorch)?
Incidentally, I am not sure we state the dependence of beanmachine on pytorch clearly enough in the overall documentation, so, it could well be that this information does need to be delivered on the website, but probably some be done somewhere else. ndmlny-qs commented on 2021-10-08T17:23:17Z I've removed the bullet points for Bean Machine and PyTorch from our template. I'll make sure to update the other tutorials, while Rob will fix this one. wtaha commented on 2021-10-08T17:42:54Z Super! Thank you! |
|
View / edit / reply to this conversation on ReviewNB wtaha commented on 2021-10-07T19:33:37Z For what it's worth, I don't see the BokehJS output above or in the rest of the doc for that matter. zaxtax commented on 2021-10-07T19:57:58Z Oh no! I think the linter I used stripped the plots out. wtaha commented on 2021-10-08T15:36:19Z Images are starting to appear, but this particular one is still not showing up.
BTW, FYI: I don't seem to get notifications, so, please ping directly if it takes too long before I follow up. zaxtax commented on 2021-10-08T16:07:50Z Loading BokehJS is the image :)
So if this is the only one not showing up that should be good enough. wtaha commented on 2021-10-08T16:43:31Z Super! BTW: I got an email update on your last message, so, it looks like this is happening! |
|
Oh no! I think the linter I used stripped the plots out. View entire conversation on ReviewNB |
|
Images are starting to appear, but this particular one is still not showing up.
BTW, FYI: I don't seem to get notifications, so, please ping directly if it takes too long before I follow up. View entire conversation on ReviewNB |
|
Loading BokehJS is the image :)
So if this is the only one not showing up that should be good enough. View entire conversation on ReviewNB |
|
Super! BTW: I got an email update on your last message, so, it looks like this is happening! View entire conversation on ReviewNB |
|
You should get an email just like commenting in GH. View entire conversation on ReviewNB |
|
I've removed the bullet points for Bean Machine and PyTorch from our template. I'll make sure to update the other tutorials, while Rob will fix this one. View entire conversation on ReviewNB |
|
I have started getting these emails now, so, I think we are all set! View entire conversation on ReviewNB |
|
Super! Thank you! View entire conversation on ReviewNB |
|
@wtaha, I believe the significance of your concern depends on the skill level of the intended audience. For novices, explicit learning outcomes are really helpful (actually, I'd say they're useful to experts as well; they tell me whether I can skip a whole tutorial outright). IMHO, the benefits of including explicit learning outcomes outweigh this concern of making the text slightly longer. I think including learning outcomes in this way doesn't slow down a user meaningfully—skimming or skipping a bulleted list is not a big deal. If the concern is that the notebooks are too long, we can edit the prose judiciously.
So, given the points above (and that including learning outcomes within tutorials is consistent with the template we've been applying to all the tutorials), I suggest we leave these outcomes as they are. View entire conversation on ReviewNB |
|
Hello @dhavide, thank you for sharing your perspective on this section. It's also great that you have brought up the template. We also had one when we started working on the tutorials a year ago, and it will definitely be good to have one for the set of tutorials that we end up with. I am happy for you drive that process. View entire conversation on ReviewNB |
|
P.S. The problem section was part of the template we had for tutorials. View entire conversation on ReviewNB |
facebook-github-bot
added
the
CLA Signed
tutorials/utils/__init__.py
Outdated
| @@ -0,0 +1,10 @@ | |||
| import sys | |||
There was a problem hiding this comment.
Please add to this file:
# flake8: noqa
This should prevent all the FLAKE8 unused import warnings.
| @@ -0,0 +1,478 @@ | |||
| """Data ETL for the NBA item response tutorial.""" | |||
There was a problem hiding this comment.
There are tons of formatting violations in this file because your code does not conform to the BLACK linter. Can you please format this file with BLACK? Formatting violations prevent the change from being landed.
tutorials/plots.py
Outdated
| @@ -0,0 +1,222 @@ | |||
| from typing import List, Tuple, Union | |||
There was a problem hiding this comment.
There are tons of formatting violations in this file because your code does not conform to the BLACK linter. Can you please format this file with BLACK? Formatting violations prevent the change from being landed.
tutorials/plots.py
Outdated
| figure_kwargs: dict = dict(), | ||
| plot_kwargs: dict = dict(), |
There was a problem hiding this comment.
Please don't use parametric types without their parameters. (So, it should be dict[SomeType].)
There was a problem hiding this comment.
Also (very important):
Do not use mutable data structures for argument defaults. They are created during function definition time. All calls to the function reuse this one instance of that data structure, persisting changes between them.
tutorials/plots.py
Outdated
| figure_kwargs: dict = {}, | ||
| plot_kwargs: dict = {}, |
There was a problem hiding this comment.
Please don't use parametric types without their parameters. (So, the type annotation should be dict[SomeType].)
There was a problem hiding this comment.
Also (very important):
Do not use mutable data structures for argument defaults. They are created during function definition time. All calls to the function reuse this one instance of that data structure, persisting changes between them.
tutorials/plots.py
Outdated
| plot_sources: List[ColumnDataSource], | ||
| labels: List[str], | ||
| colors: List[str], | ||
| figure_kwargs: dict = dict(), |
There was a problem hiding this comment.
Please don't use parametric types without their parameters. (So, the type annotation should be dict[SomeType].)
There was a problem hiding this comment.
Also (very important):
Do not use mutable data structures for argument defaults. They are created during function definition time. All calls to the function reuse this one instance of that data structure, persisting changes between them.
|
@michaeltingley I am not sure why you are getting black warnings as I ran the notebooks through a linter. |
|
@feynmanliang @michaeltingley @wtaha Are there any unaddressed issues in the notebook itself? |
|
This is a cool model! I just left a few inline comments around plotting. A bit of exposition on selecting the priors in this model would be helpful too. |
|
@jpchen has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
Motivation
Please describe your motivation for the changes. Provide link to any related issues.
Changes proposed
Outline the proposed changes and alternatives considered.
Test Plan
Please provide clear instructions on how the changes were verified. Attach screenshots if applicable.
Types of changes
Checklist