Skip to content

MAINT Parameters validation for covariance.oas #24904

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 14 commits into from
Dec 28, 2022
Merged

MAINT Parameters validation for covariance.oas #24904

merged 14 commits into from
Dec 28, 2022

Conversation

raghuveerbhat
Copy link
Contributor

@raghuveerbhat raghuveerbhat commented Nov 12, 2022 •
edited
Loading

Reference Issues/PRs

Towards #24862
Also linked to #24868

What does this implement/fix? Explain your changes.

Refactor covariance.oas as parameter validation is happening inside the OAS class.

Any other comments?

I think 1d arrays for X is deprecated. If that is indeed the case then maybe I can remove the X.ndim == 1 check in _oas().

@glemaitre
Copy link
Member

It looks good. codecov is complaining. It could be the right time to add the missing test to be sure that we raise the warning properly and cover the lines.

@raghuveerbhat
Copy link
Contributor Author

Hi @glemaitre , thank you for the review. I've some doubts when i was writing unit test for the changes. Please let me know if the comments highlighted above makes sense. I'll modify the code according to your response :)

raghuveerbhat
raghuveerbhat commented Nov 17, 2022
View reviewed changes
sklearn/covariance/_shrunk_covariance.py
X = X - X.mean()
return np.atleast_2d((X**2).mean()), 0.0

n_samples, n_features = X.shape
Copy link
Contributor Author

@raghuveerbhat raghuveerbhat Nov 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I removed the following check as this is moved inside the class

::Removed::

- if X.ndim == 1:
-     n_samples = 1
-     n_features = X.size
- else:
-     n_samples, n_features = X.shape

::Addition::

+     n_samples, n_features = X.shape

Please let me know if this is okay.

@raghuveerbhat
Copy link
Contributor Author

raghuveerbhat commented Nov 17, 2022 •
edited
Loading

Hi @glemaitre , some checks are failing and I think this is because we are now allowing 1d arrays to be passed and not raising Value error which was the existing behaviour (commit - 63cc0b6). Please let me know if any changes needs to be done, I'll do it.

@jeremiedbb jeremiedbb added the Validation related to input validation label Nov 18, 2022
@raghuveerbhat
Copy link
Contributor Author

Hi @glemaitre , I've fixed the ci errors :)

@glemaitre glemaitre removed their request for review December 1, 2022 14:14
jeremiedbb
jeremiedbb approved these changes Dec 27, 2022
View reviewed changes
Copy link
Member

@jeremiedbb jeremiedbb left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @raghuveerbhat

@jeremiedbb jeremiedbb merged commit a4d4708 into scikit-learn:main Dec 28, 2022
jjerphan pushed a commit to jjerphan/scikit-learn that referenced this pull request Jan 3, 2023
@raghuveerbhat @glemaitre
@jeremiedbb @jjerphan
MAINT Parameters validation for covariance.oas (scikit-learn#24904)
3910641 
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: jeremiedbb <jeremiedbb@yahoo.fr>
jjerphan pushed a commit to jjerphan/scikit-learn that referenced this pull request Jan 20, 2023
@raghuveerbhat @glemaitre
@jeremiedbb @jjerphan
MAINT Parameters validation for covariance.oas (scikit-learn#24904)
b815369 
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: jeremiedbb <jeremiedbb@yahoo.fr>
jjerphan pushed a commit to jjerphan/scikit-learn that referenced this pull request Jan 20, 2023
@raghuveerbhat @glemaitre
@jeremiedbb @jjerphan
MAINT Parameters validation for covariance.oas (scikit-learn#24904)
430b620 
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
Co-authored-by: jeremiedbb <jeremiedbb@yahoo.fr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@glemaitre glemaitre glemaitre left review comments

@jeremiedbb jeremiedbb jeremiedbb approved these changes

Assignees
No one assigned
Labels
module:covariance No Changelog Needed Validation related to input validation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@raghuveerbhat @glemaitre @jeremiedbb