Skip to content

Conversation

vincentschen
Copy link
Member

@vincentschen vincentschen commented Mar 23, 2020

Description of proposed changes

  • Move predict_proba to BaseLabeler class as abtractmethod
  • Move predict, score, save, and load methods to BaseLabeler class
    as shared methods in parent class
  • Update RandomVoter, MajorityClassVoter, MajorityLabelVoter, and
    LabelModel to be subclasses of BaseLabeler.
  • Update docstrings to reflect abstract/parent class hierarchy.
  • NOTE: We re-define methods in the LabelModel(BaseLabeler) subclass for predict and score in order to explicitly show docstring examples

Test plan

  • Existing tests pass: Ran tox and tox -e complex.

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.

@vincentschen vincentschen requested review from bhancock8 and a team March 23, 2020 06:16
@codecov
Copy link

codecov bot commented Mar 23, 2020

Codecov Report

Merging #1559 into master will increase coverage by 0.01%.
The diff coverage is 97.56%.

@@            Coverage Diff             @@
##           master    #1559      +/-   ##
==========================================
+ Coverage   97.11%   97.13%   +0.01%     
==========================================
  Files          55       56       +1     
  Lines        2079     2091      +12     
  Branches      342      342              
==========================================
+ Hits         2019     2031      +12     
  Misses         31       31              
  Partials       29       29              
Impacted Files Coverage Δ
snorkel/labeling/model/base_labeler.py 96.96% <96.96%> (ø)
snorkel/labeling/model/baselines.py 100.00% <100.00%> (+2.85%) ⬆️
snorkel/labeling/model/label_model.py 95.54% <100.00%> (-0.26%) ⬇️

* Move predict_proba to BaseLabeler class as abtractmethod
* Move predict, score, save, and load methods to BaseLabeler class
as shared methods in parent class
* Update RandomVoter, MajorityClassVoter, MajorityLabelVoter, and
LabelModel to be subclasses of BaseLabeler.
@vincentschen vincentschen force-pushed the refactor-base-labeler branch from 33d0de9 to 0b08e5b Compare March 23, 2020 18:52
@bhancock8
Copy link
Member

The logic all looks good. The only thing that bugs me is having predict() and score() being strictly pass-through methods on LabelModel. The duplicated passing of arguments and documentation invites staleness. The documentation demonstrating usage should be covered by unit tests anyway. I'd vote we just let the LabelModel inherit those methods from its parent.

@bhancock8
Copy link
Member

Discussed IRL. Decision to leave the methods on LabelModel to keep the Examples section for the documentation.

@vincentschen vincentschen merged commit dd24000 into master Mar 23, 2020
@vincentschen vincentschen deleted the refactor-base-labeler branch March 23, 2020 22:26
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.

2 participants