ENH Remove hardcoded device choice in _weighted_sum #27232
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ogrisel
reviewed
Sep 5, 2023
Some Array API compatible libraries do not have a device called 'cpu'. Instead we try and detect the lib+device combination that does not support float64.
0e9306f to
4cd8bd9
Compare
4cd8bd9 to
0576ec2
Compare
This was referenced Sep 29, 2023
thomasjpfan
reviewed
Dec 6, 2023
There was a problem hiding this comment.
For my education: why do we convert the type to float if it isn't?
From reading the code there are two issues:
sample_score = sample_score / scalewill break in Array API for ints because it does not automatically promote to floatsxp.sum(sample_score)can overflow more easily with ints.
|
I think this has been addressed in #27904 and can be closed ? |
|
Thanks for bumping this Franck. Closing! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this implement/fix? Explain your changes.
Some Array API compatible libraries do not have a device called 'cpu'. Instead to deal with the fact that some devices (e.g. MPS) do not have
float64support, we try and detect the lib+device combination that does not supportfloat64.I also made it so that if weights are passed that array is passed to
get_namespaceso that both arrays have to be in the same namespace.In general there is no generic way of specifying a device that all libraries understand. This means the string
'cpu'might or might not be understood. In addition there are libraries like cupy that do not even have a CPU device (no matter what you call it).I was also wondering what would happen if we have the scores on the CPU, but the weights on a different device.
Any other comments?
This is related to/inspired by reading/xref #27137. That PR also adds tests for
_weighted_sumwhich is why I've not added any here.For my education: why do we convert the type to float if it isn't? For example if we are doing a sum of an array of integers, without weights, then it seems more natural to stay with
ints (to avoid the problem that someints can't be represented by floats, etc).I think this shows that we need to figure out a solution to the lack of CI :-/