-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
FIX Make DistanceMetrics support readonly buffers attributes #21694
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
Conversation
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Jérémie du Boisberranger <jeremiedb>
Failing CI jobs for documentation are being addressed by: #21698. |
There was a problem hiding this 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.
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks @jjerphan !
Good to see that |
Reference Issues/PRs
Relates to #21685.
What does this implement/fix? Explain your changes.
03aa496#diff-8440f74258fda86e966d2b78a62c660ece7c2385ac7a55d0e40d552a492192c3R54-R55 converted the
vec
andmat
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.