Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix for #1296 #1312

Merged
merged 3 commits into from
Jul 23, 2024
Merged

Fix for #1296 #1312

merged 3 commits into from
Jul 23, 2024

Conversation

aPovidlo
Copy link
Collaborator

This is a 🐛 bug fix.

Summary

There was not enough copying for encoded categorical features during data spliting.

Fixes #1296

@aPovidlo aPovidlo requested a review from DRMPN July 22, 2024 15:53
Copy link
Contributor

github-actions bot commented Jul 22, 2024

All PEP8 errors has been fixed, thanks ❤️

Comment last updated at

Copy link

codecov bot commented Jul 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.94%. Comparing base (e0b4ee7) to head (8cbb7cb).
Report is 19 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1312   +/-   ##
=======================================
  Coverage   79.93%   79.94%           
=======================================
  Files         146      146           
  Lines       10097    10100    +3     
=======================================
+ Hits         8071     8074    +3     
  Misses       2026     2026           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@DRMPN DRMPN left a comment

Choose a reason for hiding this comment

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

Думаю, что стоит добавить тест для этого случая.

@DRMPN DRMPN requested a review from Lopa10ko July 22, 2024 16:12
Copy link
Collaborator

@Lopa10ko Lopa10ko left a comment

Choose a reason for hiding this comment

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

lgtm

можно вместо нового теста при возможности добавить сценарий к уже существующим в test/unit/data/test_data_split

@aPovidlo aPovidlo requested review from DRMPN and Lopa10ko July 23, 2024 12:29
@aPovidlo aPovidlo merged commit a7e4243 into master Jul 23, 2024
7 checks passed
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: ValueError: [...] the array at index 0 has size 894365 and the array at index 1 has size 1117957
3 participants