Skip to content

Conversation

@tamarl08
Copy link
Contributor

@tamarl08 tamarl08 commented May 22, 2023

  • Rename method from split_by_ids to split and edit docstring to explicitly say that the split is done on indices and not id values
  • Fix the case where split2 is not specified to use indices and not ids
  • Add unittests

This PR closes #727.

@tamarl08 tamarl08 linked an issue May 22, 2023 that may be closed by this pull request
@codecov
Copy link

codecov bot commented May 22, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.08 🎉

Comparison is base (9f26caf) 95.16% compared to head (7a4051d) 95.24%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #731      +/-   ##
==========================================
+ Coverage   95.16%   95.24%   +0.08%     
==========================================
  Files          29       29              
  Lines        3451     3449       -2     
==========================================
+ Hits         3284     3285       +1     
+ Misses        167      164       -3     
Impacted Files Coverage Δ
skll/data/featureset.py 91.57% <100.00%> (+1.57%) ⬆️
skll/experiments/utils.py 93.16% <100.00%> (ø)
skll/learner/utils.py 93.73% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Collaborator

@desilinguist desilinguist left a comment

Choose a reason for hiding this comment

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

Just a couple of minor tweaks. Looks great! Thank you for adding the comprehensive tests!!

@desilinguist desilinguist merged commit a3f45d5 into main May 23, 2023
@delete-merged-branch delete-merged-branch bot deleted the bugfix-727/split-by-ids branch May 23, 2023 13:50
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.

Inconsistent use of ids in FeatureSet.split_by_ids

4 participants