Sitelet https://github.com/EducationalTestingService/skll/pull/731
Skip to content

Bug fix in feature set split method - #731

Merged
desilinguist merged 3 commits into
mainfrom
bugfix-727/split-by-ids
May 23, 2023
Merged

desilinguist merged 3 commits into
mainfrom
bugfix-727/split-by-ids

Conversation

@tamarl08

@tamarl08 tamarl08 commented May 22, 2023 •

Copy link
Copy Markdown
Contributor
  • 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

codecov Bot commented May 22, 2023 •

Copy link
Copy Markdown

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.

@desilinguist desilinguist left a comment

Copy link
Copy Markdown
Collaborator

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!!

Comment thread skll/data/featureset.py Outdated
Comment thread tests/test_featureset.py Outdated
@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

3 participants