Skip to content

Conversation

@tramora
Copy link
Collaborator

@tramora tramora commented Feb 3, 2026

  • KhiopsClassifier, KhiopsRegressor and KhiopsEncoder

Fixes #533


TODO Before Asking for a Review

  • Rebase your branch to the latest version of main (or main-v10)
  • Make sure all CI workflows are green
  • When adding a public feature/fix: Update the Unreleased section of CHANGELOG.md (no date)
  • Self-Review: Review "Files Changed" tab and fix any problems you find
  • API Docs (only if there are changes in docstrings, rst files or samples):
    • Check the docs build without warning: see the log of the API Docs workflow
    • Check that your changes render well in HTML: download the API Docs artifact and open index.html
    • If there are any problems it is faster to iterate by building locally the API Docs

type_error_message("n_feature_parts", self.n_feature_parts, int)
)
if self.n_feature_parts < 0:
raise ValueError("'n_feature_parts' must be positive")
Copy link
Member

Choose a reason for hiding this comment

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

positive -> non-negative

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You are totally right, this reformulation would be more precise. Unfortunatly the message is duplicated many times and for consistency sake it would be required to modify everywhere (in a distinct PR)

Copy link
Member

Choose a reason for hiding this comment

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

A commit for fixing this would suffice. It is a small change for a PR.

- KhiopsClassifier, KhiopsRegressor and KhiopsEncoder
@tramora tramora force-pushed the 533-add-max-parts-supervised-estimators branch from bef0798 to d084a67 Compare February 4, 2026 10:59
"specific_pairs": [("age", "race")],
"all_possible_pairs": False,
"construction_rules": ["TableMode", "TableSelection"],
"max_parts": 0,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure in this testcase we need to test that the default value provided to the Core API is correct or unchanged via sklearn. Indeed, in this testcase we test that the parameters are correctly passed from the sklearn code to the core API.

Copy link
Collaborator

@popescu-v popescu-v left a comment

Choose a reason for hiding this comment

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

See the comment.

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.

Support for max_parts in sklearn supervised estimators

3 participants