Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[python-package] simplify Dataset._compare_params_for_warning() #6120

Merged
merged 2 commits into from
Oct 3, 2023

Conversation

jameslamb
Copy link
Collaborator

Contributes to #3756.

Private method Dataset._compare_for_warning() is used to avoid raising unnecessary warnings when creating a Dataset from another Dataset.

It's only used in one place:

if params != reference_params:
if not self._compare_params_for_warning(
params=params,
other_params=reference_params,
ignore_keys=_ConfigAliases.get("categorical_feature")
):
_log_warning('Overriding the parameters from Reference Dataset.')

And that place only passes in results from Dataset.get_params().

Dataset.get_params() allways returns a dictionary (never None)...

def get_params(self) -> Dict[str, Any]:

... so code paths in Dataset._compare_for_warning() to handle None are unnecessary.

This PR proposes removing them.

Notes for Reviewers

I found this by checking the statement coverage of the Python package's unit tests, like this:

sh build-python.sh install

SITE_PACKAGE_DIR=$(
    python -c "import site; print(site.getsitepackages()[0])"
)
pytest \
    --cov=${SITE_PACKAGE_DIR}/lightgbm \
    --verbose \
    tests/python_package_test/

coverage html
open ./htmlcov/index.html

Found through that that these if params is None blocks are not covered by any tests, and that was my clue that they might not be necessary at all.

@jmoralez
Copy link
Collaborator

jmoralez commented Oct 2, 2023

We should update the docstring as well

@jameslamb
Copy link
Collaborator Author

We should update the docstring as well

ah yep, sorry about that.

updated in 63c38c0

@jameslamb jameslamb merged commit f175ceb into master Oct 3, 2023
41 checks passed
@jameslamb jameslamb deleted the python/simplify-compare branch October 3, 2023 04:08
Copy link

github-actions bot commented Jan 3, 2024

This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants