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

Modify test to permit higher cardinality cases for label model #1637

Merged
merged 1 commit into from
Mar 13, 2021

Conversation

fredsala
Copy link
Contributor

Description of proposed changes

The test_label_model_basic and test_label_model_sparse tests are specific to binary class; increasing the cardinality of the problem beyond 2 (which increases the number of parameters) prevents the tests from passing.

This change modifies the tests to use average error (which is invariant to the number of parameters) instead of maximum error (which makes higher-cardinality tests increasingly difficult).

Related issue(s)

Fixes #1631

Test plan

I ran the scenario in #1631 with cardinality 3,4,...,10 and the tests pass.

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.

Copy link
Member

@bhancock8 bhancock8 left a comment

Choose a reason for hiding this comment

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

Since this is a non-trivial computation, let's either update the name of the variable to something more explicit or add a comment on what exactly we're calculating. Then lgtm!

@codecov
Copy link

codecov bot commented Mar 10, 2021

Codecov Report

Merging #1637 (70d7208) into master (1adccb6) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1637   +/-   ##
=======================================
  Coverage   97.31%   97.31%           
=======================================
  Files          68       68           
  Lines        2157     2157           
  Branches      348      348           
=======================================
  Hits         2099     2099           
  Misses         30       30           
  Partials       28       28           

@fredsala fredsala force-pushed the FS-high-card-test-fix branch from 494fb1a to 9065090 Compare March 12, 2021 04:56
@fredsala fredsala force-pushed the FS-high-card-test-fix branch from 9065090 to 70d7208 Compare March 13, 2021 02:56
@fredsala
Copy link
Contributor Author

Ready for merge.

@bhancock8 bhancock8 merged commit b3b0669 into snorkel-team:master Mar 13, 2021
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.

TestLabelModelAdvanced fails when changing cardinality
2 participants