-
Notifications
You must be signed in to change notification settings - Fork 860
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
Documentation issues #1592
Documentation issues #1592
Conversation
It's failing because of errors unrelated to my changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @TrigonaMinima, thanks for the documentation improvement! Just see the two comments, happy to answer any questions.
Arguments for changing train config defaults | ||
Arguments for changing train config defaults. | ||
|
||
n_epochs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's point to a page in the documentation (https://snorkel.readthedocs.io/) instead. In case these options update, we don't want to have to remember to change them in two places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, I couldn't find this config anywhere in the snorkel docs that's why I added it here. Could you point me to the page where it is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great point, it looks like the config isn't included in documentation. This looks like a good place to add it!
@@ -825,6 +857,7 @@ def fit( | |||
>>> label_model.fit(L) | |||
>>> label_model.fit(L, Y_dev=Y_dev) | |||
>>> label_model.fit(L, class_balance=[0.7, 0.3]) | |||
>>> label_model.fit(L, n_epochs=200, lr=0.05, l2=0.4, seed=2020) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's add one of these keyword arguments to one of the above calls as a demonstration instead so that this doctest isn't too complex.
I'll take a look at the errors in CI as well. Looks like a version change for our linting tools. |
Regarding linting errors, one error was because someone has created the f strings where it's not required. I can fix them in this pull itself if it's fine. |
@TrigonaMinima it looks like there are other linting issues due to a third party package update (possibly mypy itself). Looking into this today! |
Same linting errors unrelated to my changes. |
@TrigonaMinima just landed linting fixes, so re-running your checks now! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me once tests pass!
Codecov Report
@@ Coverage Diff @@
## master #1592 +/- ##
==========================================
+ Coverage 97.13% 97.17% +0.04%
==========================================
Files 56 67 +11
Lines 2091 2126 +35
Branches 342 342
==========================================
+ Hits 2031 2066 +35
Misses 31 31
Partials 29 29
|
@TrigonaMinima if you're happy with the changes, I'll go ahead and merge this in |
@henryre yes we can go ahead with it. |
Description of proposed changes
Added documentation updates for the following:
Related issue(s)
Fixes #1585
Test plan
Not applicable
Checklist
Need help on these? Just ask!
tox -e complex
and/ortox -e spark
if appropriate.