Skip to content

Validate max_num_features is positive in plot_importance - #12395

Open
Nityahapani wants to merge 1 commit into
dmlc:masterfrom
Nityahapani:fix/plot-importance-max-num-features-validation
Open

Validate max_num_features is positive in plot_importance#12395
Nityahapani wants to merge 1 commit into
dmlc:masterfrom
Nityahapani:fix/plot-importance-max-num-features-validation

Conversation

@Nityahapani

Copy link
Copy Markdown
Contributor

Validate max_num_features is positive in plot_importance

Summary

plot_importance() implements the "top N features" selection via Python
negative-index slicing:

tuples = sorted(tuples, key=lambda _x: _x[1])[-max_num_features:]

This has two undocumented, confusing edge cases:

  • max_num_features=0: Python's [-0:] is the same as [0:], i.e. the
    entire list — so 0 silently means "show all features", the opposite
    of what a reasonable reading of "maximum number of top features
    displayed" (the parameter's own docstring) would suggest.
  • max_num_features<0: [-(-N):] becomes [N:], which skips the
    first N lowest-importance entries after sorting ascending, i.e. it
    shows the least important features rather than raising an error or
    showing the top ones. E.g. max_num_features=-3 on 5 features shows the
    2 least important ones.

Neither case has any sensible interpretation given the parameter's
documented meaning ("Maximum number of top features displayed"), so
there's no reasonable value for a user to fall back on — an explicit error
is the right behavior.

Change

     tuples = [(k, importance[k]) for k in importance]
     if max_num_features is not None:
-        # pylint: disable=invalid-unary-operand-type
+        if max_num_features <= 0:
+            raise ValueError(
+                "`max_num_features` must be a positive integer, got "
+                f"{max_num_features}. Note that due to how the top-N "
+                "selection is implemented, 0 or a negative value does not "
+                "mean 'show no features' -- it previously produced confusing, "
+                "undocumented results (0 silently showed all features; "
+                "negative values showed the *least* important features "
+                "instead of the most important ones)."
+            )
         tuples = sorted(tuples, key=lambda _x: _x[1])[-max_num_features:]
     else:
         tuples = sorted(tuples, key=lambda _x: _x[1])

This also removes the existing # pylint: disable=invalid-unary-operand-type
comment, which was suppressing pylint's (correct) inability to prove
max_num_features was numeric before negating it — the new explicit check
makes that provably safe, and I confirmed pylint no longer flags it once
the suppression comment is removed.

Verification

  • None, and positive ints (1, 5, 100): unchanged, valid
  • 0, -1, -3: now raise ValueError with a clear message instead of
    silently producing wrong results

mypy, ruff, and pylint (specifically re-checked for
invalid-unary-operand-type) all pass on the modified file.

plot_importance() implements the "top N features" selection via
Python negative-index slicing:

    tuples = sorted(tuples, key=lambda _x: _x[1])[-max_num_features:]

This has two undocumented, confusing edge cases:

  - max_num_features=0: Python's [-0:] is the same as [0:], i.e. the
    *entire* list -- so 0 silently means "show all features", the
    opposite of what a reasonable reading of "maximum number of top
    features displayed" (the parameter's own docstring) would suggest.

  - max_num_features<0: [-(-N):] becomes [N:], which skips the first
    N *lowest*-importance entries after sorting ascending, i.e. it
    shows the *least* important features rather than raising an error
    or showing the top ones. E.g. max_num_features=-3 on 5 features
    shows the 2 *least* important ones.

Neither case has any sensible interpretation given the parameter's
documented meaning ("Maximum number of top features displayed"), so
this adds explicit validation raising a clear ValueError for
max_num_features <= 0, explaining exactly why 0/negative doesn't do
what a user would likely expect from it.

This also removes the existing `# pylint: disable=invalid-unary-
operand-type` comment, which was suppressing pylint's (correct)
inability to prove max_num_features was numeric before negating it --
the new explicit check makes that provably safe, confirmed pylint no
longer flags it once the suppression comment is removed.

Verified standalone (import xgboost requires the compiled C++ core,
which isn't buildable in this sandbox):
  - None, and positive ints (1, 5, 100): unchanged, valid
  - 0, -1, -3: now raise ValueError with a clear message instead of
    silently producing wrong results

mypy, ruff, and pylint (specifically re-checked for
invalid-unary-operand-type) all pass on the modified file.
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.

1 participant