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

Documentation issues #1592

Merged
merged 3 commits into from
Jul 9, 2020
Merged

Documentation issues #1592

merged 3 commits into from
Jul 9, 2020

Conversation

TrigonaMinima
Copy link
Contributor

Description of proposed changes

Added documentation updates for the following:

  • List of possible metrics available while scoring
  • Model training parameters available

Related issue(s)

Fixes #1585

Test plan

Not applicable

Checklist

Need help on these? Just ask!

  • I have read the CONTRIBUTING document.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • I have run tox -e complex and/or tox -e spark if appropriate.
  • All new and existing tests passed.

@TrigonaMinima
Copy link
Contributor Author

It's failing because of errors unrelated to my changes.

Copy link
Member

@henryre henryre left a 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
Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

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)
Copy link
Member

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.

@henryre
Copy link
Member

henryre commented Jun 16, 2020

I'll take a look at the errors in CI as well. Looks like a version change for our linting tools.

@TrigonaMinima
Copy link
Contributor Author

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.

@henryre
Copy link
Member

henryre commented Jun 20, 2020

@TrigonaMinima it looks like there are other linting issues due to a third party package update (possibly mypy itself). Looking into this today!

@TrigonaMinima
Copy link
Contributor Author

Same linting errors unrelated to my changes.

@henryre
Copy link
Member

henryre commented Jun 20, 2020

@TrigonaMinima just landed linting fixes, so re-running your checks now!

@henryre henryre closed this Jun 20, 2020
@henryre henryre reopened this Jun 20, 2020
Copy link
Member

@henryre henryre left a 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
Copy link

codecov bot commented Jun 20, 2020

Codecov Report

Merging #1592 into master will increase coverage by 0.04%.
The diff coverage is n/a.

@@            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              
Impacted Files Coverage Δ
snorkel/labeling/model/label_model.py 95.54% <ø> (ø)
snorkel/labeling/model/__init__.py 100.00% <0.00%> (ø)
snorkel/utils/__init__.py 100.00% <0.00%> (ø)
snorkel/preprocess/__init__.py 100.00% <0.00%> (ø)
snorkel/slicing/__init__.py 100.00% <0.00%> (ø)
snorkel/labeling/__init__.py 100.00% <0.00%> (ø)
...norkel/classification/training/loggers/__init__.py 100.00% <0.00%> (ø)
snorkel/map/__init__.py 100.00% <0.00%> (ø)
snorkel/classification/__init__.py 100.00% <0.00%> (ø)
snorkel/slicing/sf/__init__.py 100.00% <0.00%> (ø)
... and 3 more

@henryre
Copy link
Member

henryre commented Jun 20, 2020

@TrigonaMinima if you're happy with the changes, I'll go ahead and merge this in

@TrigonaMinima
Copy link
Contributor Author

@henryre yes we can go ahead with it.

@henryre henryre merged commit a55bb6f into snorkel-team:master Jul 9, 2020
@TrigonaMinima TrigonaMinima deleted the docs branch July 13, 2020 04:38
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.

List of inbuilt metrics in score page of label model
2 participants