Skip to content

Conversation

@DeborahOlaboye
Copy link

@DeborahOlaboye DeborahOlaboye commented Jan 31, 2026

Contributes to #7031 by expanding test coverage for plotting module parameters.

New tests added:

Item from #7031 Function Parameter
plot_importance() max_num_features
plot_split_value_histogram() xlim
plot_metric() xlim

Test plan

  • pytest tests/python_package_test/test_plotting.py::test_plot_importance passes
  • pytest tests/python_package_test/test_plotting.py::test_plot_split_value_histogram passes
  • pytest tests/python_package_test/test_plotting.py::test_plot_metrics passes

@DeborahOlaboye
Copy link
Author

@DeborahOlaboye please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.

@microsoft-github-policy-service agree [company="{your company}"]

Options:

  • (default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
  • (when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"

Contributor License Agreement

@microsoft-github-policy-service agree

Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

Please revert all the unrelated formatting changes you made in this PR, and the test cases on ylim and figsize that are already being handled in #7142 and #7143 .

When you do that, please also update your branch to the latest master.

After that, we'll be happy to review again.

@DeborahOlaboye
Copy link
Author

Please revert all the unrelated formatting changes you made in this PR, and the test cases on ylim and figsize that are already being handled in #7142 and #7143 .

When you do that, please also update your branch to the latest master.

After that, we'll be happy to review again.

Alright, all unrelated formatting changes has been reverted and branch updated to the latest master. Thank you.

Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! I left some suggestions for making these tests a bit stronger.

@jameslamb jameslamb changed the title [python-package] Add tests for plotting xlim/ylim and max_num_features parameters [python-package] Add tests for some plotting functions Feb 1, 2026
@jameslamb
Copy link
Collaborator

I've updated the description and title, removing references to tests that you've since removed.

It would be ok to add back ylim and figsize tests for functions that are not plot_importance() if you'd like... all I meant in #7144 (review) is that those other PRs were already adding tests on plot_importance(). You could also do this in a follow-up PR, whichever you'd like.

@DeborahOlaboye DeborahOlaboye force-pushed the add-plotting-xlim-ylim-tests branch from 1b55e96 to 6e14ae1 Compare February 1, 2026 07:33
@DeborahOlaboye
Copy link
Author

I've updated the description and title, removing references to tests that you've since removed.

It would be ok to add back ylim and figsize tests for functions that are not plot_importance() if you'd like... all I meant in #7144 (review) is that those other PRs were already adding tests on plot_importance(). You could also do this in a follow-up PR, whichever you'd like.

Thanks for clarifying and for updating the title. I'll add ylim and figsize tests for plot_metric() and plot_split_value_histogram() in a follow-up PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants