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

API dim to n_dim in make_sparse_spd_matrix #27718

Merged
merged 14 commits into from Nov 8, 2023

Conversation

adam2392
Copy link
Contributor

@adam2392 adam2392 commented Nov 3, 2023

Reference Issues/PRs

Fixes: #27669

What does this implement/fix? Explain your changes.

Deprecates dim in place of n_dim.

Any other comments?

Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
Copy link

github-actions bot commented Nov 3, 2023

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: 2a2ef49. Link to the linter CI: here

Signed-off-by: Adam Li <adam2392@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
@glemaitre glemaitre changed the title [API] dim to `n_dim in make_sparse_spd_matrix API dim to n_dim in make_sparse_spd_matrix Nov 3, 2023
@glemaitre glemaitre self-requested a review November 3, 2023 18:16
Signed-off-by: Adam Li <adam2392@gmail.com>
Copy link
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

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

You need to add a specific test to check the deprecation warning.
You also need a new test for checking that we raise the expected error.

I also saw some other failure, I assume that we were using the dim variable in someway.

doc/whats_new/v1.4.rst Outdated Show resolved Hide resolved
sklearn/datasets/_samples_generator.py Show resolved Hide resolved
sklearn/datasets/_samples_generator.py Outdated Show resolved Hide resolved
sklearn/datasets/_samples_generator.py Outdated Show resolved Hide resolved
),
FutureWarning,
)
n_dim = dim
Copy link
Member

Choose a reason for hiding this comment

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

do not change n_dim but _n_dim. We should raise an error if a user try to specify both n_dim and dim.

I assume that the time of the deprecation, we need to have n_dim=None to detect explicitely if a user pass n_dim=1 or not. This is annoying but I don't see how to detect this case otherwise. This would not change the default value because None would default to 1. Somehow, we can omit to document it explicitely, and make a more silent deprecation because I don't think that any user will use n_dim=Noe explicitely to set it to 1.

adam2392 and others added 5 commits November 3, 2023 14:38
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Signed-off-by: Adam Li <adam2392@gmail.com>
sklearn/datasets/_samples_generator.py Outdated Show resolved Hide resolved
sklearn/datasets/_samples_generator.py Outdated Show resolved Hide resolved
sklearn/datasets/_samples_generator.py Outdated Show resolved Hide resolved
Copy link
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

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

LGTM on my side. Thanks @adam2392

adam2392 and others added 2 commits November 4, 2023 14:12
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM

sklearn/datasets/_samples_generator.py Outdated Show resolved Hide resolved
adam2392 and others added 2 commits November 8, 2023 09:15
@thomasjpfan thomasjpfan enabled auto-merge (squash) November 8, 2023 14:29
@thomasjpfan thomasjpfan merged commit 096b525 into scikit-learn:main Nov 8, 2023
25 checks passed
@adam2392 adam2392 deleted the ndim branch November 8, 2023 15:10
REDVM pushed a commit to REDVM/scikit-learn that referenced this pull request Nov 16, 2023
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com>
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.

[DOC, API] Make dimensionality parameter the same name in make_sparse_spd_matrix and make_spd_matrix
3 participants