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

FIX Make DistanceMetrics support readonly buffers attributes #21694

Merged
merged 10 commits into from Nov 19, 2021

Conversation

jjerphan
Copy link
Member

@jjerphan jjerphan commented Nov 17, 2021

Reference Issues/PRs

Relates to #21685.

What does this implement/fix? Explain your changes.

03aa496#diff-8440f74258fda86e966d2b78a62c660ece7c2385ac7a55d0e40d552a492192c3R54-R55 converted the vec and mat ndarray into not const qualified memory views.

Hence, when creating DistanceMetric instances (e.g. when unpickling them) an error is raised when numpy array buffer are readonly, (which is a common case in framework for distributed computating).

Any other comments?

This fix is currently WIP.

An alternative fix would be to keep the introduced test but revert 03aa496.

@jjerphan jjerphan changed the title FIX Deserialization of DistanceMetrics with readonly buffers attributes FIX Make DistanceMetrics support readonly buffers attributes Nov 17, 2021
sklearn/metrics/_dist_metrics.pyx Outdated Show resolved Hide resolved
sklearn/metrics/tests/test_dist_metrics.py Outdated Show resolved Hide resolved
@jjerphan jjerphan marked this pull request as ready for review November 17, 2021 16:45
@jjerphan
Copy link
Member Author

Failing CI jobs for documentation are being addressed by: #21698.

Copy link
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

LGTM. I had a doubt on the strength of the non-regression test but all is good as it is.

Please document the fix in whats new targeting 1.0.2 as this is an important fix for a regression introduced in 1.0.0 w.r.t. 0.24.2.

sklearn/metrics/tests/test_dist_metrics.py Show resolved Hide resolved
doc/whats_new/v1.0.rst Outdated Show resolved Hide resolved
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
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 @jjerphan !

@jeremiedbb jeremiedbb merged commit 33c39e5 into scikit-learn:main Nov 19, 2021
@lorentzenchr
Copy link
Member

Good to see that ReadonlyArrayWrapper is actually helpful.

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.

None yet

4 participants