Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Issue 118 catboost #119

Closed
wants to merge 16 commits into from
Closed

Issue 118 catboost #119

wants to merge 16 commits into from

Conversation

ValeriaGomes
Copy link
Contributor

@ValeriaGomes ValeriaGomes commented Jan 14, 2020

Status

READY

Todo list

Background context

  • CatBoost has several parameters inside the fit method. One possible solution is to add a fit_params into the CatBoost function in FkLearn for increased flexibility.
  • This improvement would have to be done for both classification and regression.

This is important especially for CatBoost because the parameter that allows it to treat categorical features by itself is inside the fit method, and this is the main differential in this library.

Description of the changes proposed in the pull request

Allow the user to set fit params in CatBoost: https://catboost.ai/docs/concepts/python-reference_catboostregressor_fit.html#python-reference_catboostregressor_fit

Where should the reviewer start?

src/fklearn/training/classification.py
src/fklearn/training/regression.py

@ValeriaGomes ValeriaGomes requested a review from a team as a code owner January 14, 2020 20:48
@codecov-io
Copy link

codecov-io commented Jan 14, 2020

Codecov Report

Merging #119 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #119      +/-   ##
==========================================
+ Coverage   94.81%   94.82%   +<.01%     
==========================================
  Files          25       25              
  Lines        1582     1584       +2     
  Branches      220      220              
==========================================
+ Hits         1500     1502       +2     
  Misses         49       49              
  Partials       33       33
Impacted Files Coverage Δ
src/fklearn/training/classification.py 100% <100%> (ø) ⬆️

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 da11407...d273fff. Read the comment docs.

Copy link

@gabrielferreira95 gabrielferreira95 left a comment

Choose a reason for hiding this comment

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

Nice work!
I believe we need to test this new parameter, though. Probably, we could update this test to use the added parameter fit_params. Or even create another test.

src/fklearn/training/classification.py Outdated Show resolved Hide resolved
src/fklearn/training/regression.py Outdated Show resolved Hide resolved
@ValeriaGomes
Copy link
Contributor Author

I'm reviewing this test because it's breaking when I insert the fit_params.

@ValeriaGomes ValeriaGomes added the wip Work in progress label Jan 22, 2020
caique-lima
caique-lima previously approved these changes Feb 3, 2020
@caique-lima
Copy link
Contributor

Closing this one as we don't have updates

@gcbeltramini gcbeltramini deleted the issue-118-catboost branch March 25, 2023 00:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wip Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow fit params for CatBoost
4 participants