Skip to content

Conversation

@desilinguist
Copy link
Collaborator

This PR closes #565.

  • It also modifies the Learner._create_label_dict() argument so that it internally keeps track of whether label_dict has already been set.
  • No additional tests are necessary since the cross-validation and learning curve tests already represent scenarios where this function is called twice, once in the top-level cross_validate()/learning_curve() methods and once again when the underlying models are trained.

- Modify the `Learner._create_label_dict()` argument so that it internally keeps track of whether `label_dict` has already been set.
@desilinguist desilinguist requested review from a user, Lguyogiro, aoifecahill and mulhod May 4, 2020 19:49
@codecov
Copy link

codecov bot commented May 4, 2020

Codecov Report

Merging #605 into master will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #605      +/-   ##
==========================================
- Coverage   95.16%   95.16%   -0.01%     
==========================================
  Files          26       26              
  Lines        2978     2977       -1     
==========================================
- Hits         2834     2833       -1     
  Misses        144      144              
Impacted Files Coverage Δ
skll/learner/__init__.py 96.28% <100.00%> (-0.01%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 547fdd5...0775aca. Read the comment docs.

@aoifecahill
Copy link
Collaborator

Looks good to me. What about the drop in code coverage (1 line?). Is that just noise?

@desilinguist
Copy link
Collaborator Author

Looks good to me. What about the drop in code coverage (1 line?). Is that just noise?

Yup, we can ignore that.

Copy link
Contributor

@mulhod mulhod left a comment

Choose a reason for hiding this comment

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

Nitpicky typo suggestion. Looks good to me, though!

Co-authored-by: Matt Mulholland <mulhodm@gmail.com>
@desilinguist desilinguist merged commit a9599ea into master May 7, 2020
@delete-merged-branch delete-merged-branch bot deleted the 565-remove-create-label-dict-argument branch May 7, 2020 15:21
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.

Remove create_label_dict argument from Learner.train()

4 participants