Update Core API docstring and default values by popescu-v · Pull Request #412 · KhiopsML/khiops-python

popescu-v · 2025-05-28T17:12:09Z

replace "None" with "none" as acceptable values for discretization_method and grouping_method, following Khiops Core PR 692 use "none" rather than "None" in all khiops parameters khiops#695
use "MODL" as default value instead of Python None for the same two parameters
in train_recoder, fix documented default value of keep_initial_categorical_variables and keep_initial_numerical_variables to False, according to the function signature.

TODO Before Asking for a Review

Rebase your branch to the latest version of dev (or main for release PRs)
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

folmos-at-orange · 2025-05-28T17:27:10Z

khiops/core/api.py

            - "0-1 binarization": A 0's and 1's coding the interval/group id
            - "conditional info": Conditional information of the interval/group
            - "none": Keeps the variable as-is
-    discretization_method : str


I was wondering what "none" does.

"none" applies no discretization, nor grouping AFAIK; just basic statistics are computed.

marcboulle · 2025-05-30T07:15:19Z

khiops/core/api.py

    # Remove discretization method if specified for supervised analysis:
    # it is always MODL
    if "discretization_method" in task_args and task_args["target_variable"] != "":
        del task_args["discretization_method"]


Je ne comprend pas à quoi cela sert de supprimer la méthode de discrétisation de la cas supervisé, puisque dans ce cas, cette spécification est simplement ignorée, et que cela ne pose aucun problème.

De plus, comme la ligne suivante est dans le scénario, cela ne risque-t-il pas de faire planter le moteur de templating?
DiscretizerUnsupervisedMethodName __discretization_method__

Pourrait-on supprimer ces lignes de preprocessing des arguments des tâches?

The templating engine doesn't fail, because the expected default value (viz. "MODL") is injected automatically in the template if the argument is missing from the API call. However, in cases such as this one, this behavior leads to potentially spurious effects: the "MODL" value is injected in the scenario, even if the user had specified, say, "none".
Hence, e.g.:

the user specifies discretization_method="none" in a call to train_predictor for a supervised learning task (target_variable != "");

the _preprocess_task_arguments removes discretization_method from the arguments list;

the default value for this argument, i.e. "MODL" is injected into the scenario template as DiscretizerUnsupervisedMethodName MODL;

As there is a target specified (e.g. AnalysisSpec.TargetAttributeName class), DiscretizerUnsupervisedMethodName is ignored altogether by the MODL_* binary.

Hence, the result is correct all in all, but the behavior is unnecessarily complicated and generates spurious entries in the scenarios.

Hence, I will remove these preprocessing lines.

marcboulle · 2025-05-30T07:16:51Z

khiops/core/api.py

-
    # Remove grouping method if specified for supervised analysis: it is always MODL
    if "grouping_method" in task_args and task_args["target_variable"] != "":
        del task_args["grouping_method"]


Idem pour la méthode de groupement de valeur, avec la ligne suivante du scenario:
GrouperUnsupervisedMethodName __grouping_method__

Yes indeed, I will remove the preprocessing (see the comment above).

marcboulle · 2025-05-30T07:24:28Z

khiops/core/api.py

        Allows grouping of the target variable values in classification. It can
        substantially increase the training time.
-    discretization_method : str
+    discretization_method : str, default "MODL"


Pour info, l'info-bulle de ce champ dans la GUI est actuellement:
Name of the discretization method in case of unsupervised analysis.

Ne pourrait-on pas harmoniser, dans un sens ou dans un autre?

Note: détail, facultatif, à noter éventuellement pour plus tard de façon plus générale en complément de l'issue #363

OK, I will just update the docstring according to the Khiops Core entry.

marcboulle · 2025-05-30T07:25:05Z

khiops/core/api.py

-    grouping_method : str
+        Its valid values are: "MODL", "EqualWidth", "EqualFrequency" or "none".
+        Ignored for supervised analysis.
+    grouping_method : str, default "MODL"


Pour info, l'info-bulle de ce champ dans la GUI est actuellement:
Name of the value grouping method in case of unsupervised analysis.

Cf. commentaire précédent sur la méthode de discrétisation

OK, I will update the docstring accordingly.

- replace "None" with "none" as acceptable values for discretization_method and grouping_method, following Khiops Core PR KhiopsML/khiops#695 - use "MODL" as default value instead of Python None for the same two parameters - stop removing the discretization_method and grouping_method arguments in case of supervised analysis: they are ignored by Khiops Core in the scenarios anyway, and removing them generated spurious scenario entries (default values substituted in the templates in case of absence). - in train_recoder, fix documented default value of keep_initial_categorical_variables and keep_initial_numerical_variables to False, according to the function signature.

marcboulle

LGTM

popescu-v requested review from folmos-at-orange and marcboulle May 28, 2025 17:12

folmos-at-orange approved these changes May 28, 2025

View reviewed changes

marcboulle requested changes May 30, 2025

View reviewed changes

popescu-v force-pushed the fix-api-docstrings-and-default-values branch from 94d0c3f to 188b85f Compare June 2, 2025 09:04

popescu-v requested a review from marcboulle June 2, 2025 09:05

marcboulle approved these changes Jun 2, 2025

View reviewed changes

popescu-v merged commit e325efd into dev Jun 2, 2025
25 of 29 checks passed

popescu-v deleted the fix-api-docstrings-and-default-values branch June 2, 2025 09:53

Conversation

popescu-v commented May 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

TODO Before Asking for a Review

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcboulle May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

popescu-v Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marcboulle left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

popescu-v commented May 28, 2025 •

edited

Loading

marcboulle May 30, 2025 •

edited

Loading

popescu-v Jun 2, 2025 •

edited

Loading