Skip to content

FIX Correct the initiatialization of precisions_cholesky_ from precisions_init #26416

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 54 commits into from
Jul 20, 2023
Merged

FIX Correct the initiatialization of precisions_cholesky_ from precisions_init #26416

merged 54 commits into from
Jul 20, 2023

Conversation

mchikyt3
Copy link
Contributor

Reference Issues/PRs

Fixes #26415

What does this implement/fix? Explain your changes.

This PR fixes the bug of initializing precisions_cholesky_ from precisions_init in the _initialize method.

Any other comments?

jjerphan
jjerphan reviewed Jun 1, 2023
View reviewed changes
Copy link
Member

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

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

Thank you for reporting the issue and proposing a fix.

Your observation and your correction proposal are valid. Here are just a few comments to avoid performance regressions.

@jjerphan jjerphan changed the title BUG Fix initializing precisions_cholesky_ from precisions_init FIX Correct the initiatialization of precisions_cholesky_ from precisions_init Jun 1, 2023
@jjerphan jjerphan mentioned this pull request Jun 1, 2023
Closed
mchikyt3 and others added 9 commits June 1, 2023 19:43
@mchikyt3 @jjerphan
Update sklearn/mixture/tests/test_gaussian_mixture.py
806c7e6 
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
@mchikyt3 @jjerphan
Update sklearn/mixture/tests/test_gaussian_mixture.py
3f816ed 
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
@mchikyt3 @jjerphan
Update sklearn/mixture/tests/test_gaussian_mixture.py
17acaf4 
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
@mchikyt3 @jjerphan
Update sklearn/mixture/tests/test_gaussian_mixture.py
d172fc7 
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
@mchikyt3 @jjerphan
Update sklearn/mixture/tests/test_gaussian_mixture.py
4258ee6 
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
@mchikyt3
Merge branch 'scikit-learn:main' into fix-precisions-init
48ea831
@mchikyt3
Used np.flipup and np.fliplr to permutate precision array. Parame…
d1a7705 
…terized `COVARIANCE_TYPE`.
@mchikyt3 @jjerphan
Update doc/whats_new/v1.3.rst
49e5e6c 
Co-authored-by: Julien Jerphanion <git@jjerphan.xyz>
@mchikyt3
Typo fixed.
d3b9fd7
jjerphan
jjerphan approved these changes Jun 2, 2023
View reviewed changes
Copy link
Member

@jjerphan jjerphan left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you, @mchikyt3.

Just a few last comments.

mchikyt3 and others added 3 commits June 2, 2023 10:45
jjerphan
jjerphan reviewed Jun 2, 2023
View reviewed changes
@jjerphan jjerphan added the Waiting for Second Reviewer First reviewer is done, need a second one! label Jun 2, 2023
@mchikyt3
Copy link
Contributor Author

mchikyt3 commented Jul 11, 2023 •
edited
Loading

The change log has been moved to v1.4.rst. Thanks.

@ogrisel
Copy link
Member

ogrisel commented Jul 11, 2023

@dengemann do you want to have a look at this PR?

OmarManzoor
OmarManzoor reviewed Jul 19, 2023
View reviewed changes
Copy link
Contributor

@OmarManzoor OmarManzoor 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 the PR @mchikyt3. This looks good overall. I just pointed out some changes related to the docs and naming.

mchikyt3 and others added 9 commits July 19, 2023 19:56
@mchikyt3 @OmarManzoor
Update sklearn/mixture/_gaussian_mixture.py
a8a2ab7 
Co-authored-by: Omar Salman <omar.salman@arbisoft.com>
@mchikyt3 @OmarManzoor
Update sklearn/mixture/_gaussian_mixture.py
ead1671 
Co-authored-by: Omar Salman <omar.salman@arbisoft.com>
@mchikyt3 @OmarManzoor
Update sklearn/mixture/_gaussian_mixture.py
1e1f8d1 
Co-authored-by: Omar Salman <omar.salman@arbisoft.com>
@mchikyt3 @OmarManzoor
Update sklearn/mixture/_gaussian_mixture.py
8d38aab 
Co-authored-by: Omar Salman <omar.salman@arbisoft.com>
@mchikyt3 @OmarManzoor
Update sklearn/mixture/_gaussian_mixture.py
e8cbb98 
Co-authored-by: Omar Salman <omar.salman@arbisoft.com>
@mchikyt3 @OmarManzoor
Update sklearn/mixture/_gaussian_mixture.py
f841414 
Co-authored-by: Omar Salman <omar.salman@arbisoft.com>
@mchikyt3 @OmarManzoor
Update sklearn/mixture/_gaussian_mixture.py
4841f90 
Co-authored-by: Omar Salman <omar.salman@arbisoft.com>
@mchikyt3 @OmarManzoor
Update sklearn/mixture/_gaussian_mixture.py
521d4df 
Co-authored-by: Omar Salman <omar.salman@arbisoft.com>
@mchikyt3
Merge branch 'main' into fix-precisions-init
2d5f31c
@mchikyt3
Copy link
Contributor Author

mchikyt3 commented Jul 19, 2023 •
edited
Loading

Thanks for the PR @mchikyt3. This looks good overall. I just pointed out some changes related to the docs and naming.

The changes have been committed. Thanks for your comments. Please approve if there's no further questions. @OmarManzoor

@mchikyt3 @OmarManzoor
Update sklearn/mixture/tests/test_gaussian_mixture.py
36615ae 
Co-authored-by: Omar Salman <omar.salman@arbisoft.com>
OmarManzoor
OmarManzoor approved these changes Jul 20, 2023
View reviewed changes
Copy link
Contributor

@OmarManzoor OmarManzoor 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 @mchikyt3

@OmarManzoor OmarManzoor removed the Waiting for Second Reviewer First reviewer is done, need a second one! label Jul 20, 2023
@OmarManzoor OmarManzoor merged commit 7de59b2 into scikit-learn:main Jul 20, 2023
@mchikyt3 mchikyt3 deleted the fix-precisions-init branch July 20, 2023 10:52
punndcoder28 pushed a commit to punndcoder28/scikit-learn that referenced this pull request Jul 29, 2023
@mchikyt3 @punndcoder28
FIX Correct the initiatialization of precisions_cholesky_ from `pre…
a17a0ce 
…cisions_init` (scikit-learn#26416)
@glemaitre glemaitre mentioned this pull request Sep 18, 2023
Merged
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this pull request Sep 18, 2023
@mchikyt3 @glemaitre
FIX Correct the initiatialization of precisions_cholesky_ from `pre…
1a8dff4 
…cisions_init` (scikit-learn#26416)
jeremiedbb pushed a commit that referenced this pull request Sep 20, 2023
@mchikyt3 @jeremiedbb
FIX Correct the initiatialization of precisions_cholesky_ from `pre…
5082cbf 
…cisions_init` (#26416)
REDVM pushed a commit to REDVM/scikit-learn that referenced this pull request Nov 16, 2023
@mchikyt3 @REDVM
FIX Correct the initiatialization of precisions_cholesky_ from `pre…
d147416 
…cisions_init` (scikit-learn#26416)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jjerphan jjerphan jjerphan approved these changes

@OmarManzoor OmarManzoor OmarManzoor approved these changes

Assignees
No one assigned
Labels
module:mixture
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Incorrect initialization of GaussianMixture from precisions_init in the _initialize method
4 participants
@mchikyt3 @jjerphan @ogrisel @OmarManzoor