Skip to content

Conversation

@tramora
Copy link
Collaborator

@tramora tramora commented Feb 3, 2026

  • KhiopsClassifier, KhiopsRegressor, KhiopsEncoder

Fixes #543


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

Copy link
Member

@folmos-at-orange folmos-at-orange left a comment

Choose a reason for hiding this comment

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

Update the changelog.

- KhiopsClassifier, KhiopsRegressor, KhiopsEncoder
@tramora tramora force-pushed the 543-n-features-default-value branch from db77fe2 to 819bb4f Compare February 4, 2026 10:40
@tramora
Copy link
Collaborator Author

tramora commented Feb 4, 2026

Update the changelog.
Fixed

"field_separator": "\t",
"detect_format": False,
"header_line": True,
"max_constructed_variables": 1000,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd add n_features to the monotable tests (e.g. in test_parameter_transfer_classifier_fit_from_monotable_dataframe), in which case the expected max_constructed_variables should equal the value of n_features set in the test.

Copy link
Collaborator Author

@tramora tramora Feb 10, 2026

Choose a reason for hiding this comment

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

After exchanging with @folmos-at-orange on this PR or the other one related, the idea here is to verify the core parameter is set at its default value (1000) WITHOUT giving the optional input n_features.
Otherwise it would only be checking the correspondance between the value passed to the sklearn estimators and the value passed to the core

"field_separator": "\t",
"detect_format": False,
"header_line": True,
"max_constructed_variables": 1000,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Idem above.

"field_separator": "\t",
"detect_format": False,
"header_line": True,
"max_constructed_variables": 1000,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Idem above.

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.

A few minor changes in the tests (see the comments).

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.

Sklearn estimator n_features default value not coherent with core's max_features

3 participants