Skip to content

Fix a Khiops type guessing error when reading a numpy string dtype - #625

Open
tramora wants to merge 1 commit into
mainfrom
str_string_dtypes_sklearn
Open

tramora wants to merge 1 commit into
mainfrom
str_string_dtypes_sklearn

Conversation

@tramora

@tramora tramora commented Sep 23, 2026 •

Copy link
Copy Markdown
Collaborator
  • All the numpy string dtypes ('str' and 'string') must be considered

Fixes #607


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, markdown 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

@tramora
tramora requested a review from popescu-v September 23, 2026 17:51
Comment thread tests/test_dataset_errors.py Outdated
)
self.assertEqual(output_error_msg, expected_msg)

def test_pandas_table_column_value_must_accept_all_string_variants(self):

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.

Move the test to test_dataset_class.py, because no error / failure-mode is checked here; just normal behavior.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

Comment thread khiops/sklearn/dataset.py Outdated
khiops_type = "Numerical"
elif lower_numpy_type == "string":
# A unique test to catch both cases where lower_numpy_type is "str" or "string"
elif isinstance(numpy_type, pd.StringDtype):

@popescu-v popescu-v Sep 24, 2026 •

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.

For a minimally-invasive change, I'd do just:

elif lower_numpy_type in ["str", "string"]

Otherwise, the scope would be larger than the support Khiops is committed to (just Pandas, with its NumPy dependency, plus SciPy - for the sparse data):

isinstance(pd.StringDtype(storage="pyarrow"), pd.StringDtype) # => True
str(pd.StringDtype(storage="pyarrow")).lower() in ("string", "str") # => False, because "string[pyarrow]" not in ["string", "str"]

Adding further support (PyArrow, BigQuery) seems out of the scope of the current PR and deserves further assessment IMHO.

@tramora tramora Sep 25, 2026 •

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

You are right. I changed the test

@popescu-v popescu-v 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.

See the comments.

- All the numpy string dtypes ('str' and 'string') must be considered
@tramora
tramora force-pushed the str_string_dtypes_sklearn branch from 3a4e269 to 376fce7 Compare September 25, 2026 08:39
@tramora
tramora requested a review from popescu-v September 25, 2026 08:41

This branch has not been deployed

No deployments
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.

khiops.sklearn : text variables not used

2 participants