-
-
Notifications
You must be signed in to change notification settings - Fork 35
Avoid divide the first element for type check in normalize #1458
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
base: master
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1458 +/- ##
=======================================
Coverage 93.90% 93.90%
=======================================
Files 34 34
Lines 15938 15938
=======================================
+ Hits 14966 14967 +1
+ Misses 972 971 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| nrm = norm(a, p) | ||
| if !isempty(a) | ||
| aa = copymutable_oftype(a, typeof(first(a)/nrm)) | ||
| aa = copymutable_oftype(a, float(Base.promote_eltype(a, nrm))) |
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.
This won't work for arrays of arrays:
julia> a = [[1,2], [3,4]]; nrm = norm(a)
5.477225575051661
julia> Base.promote_eltype(a, nrm)
AnyThere 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.
Maybe instead of calling __normalize!, which requires us to compute the type of the array, we should just call map, which does the type computation for us. Something like:
function normalize(a::AbstractArray, p::Real = 2)
nrm = norm(a, p)
# duplicate logic from __normalize!, but applied to map:
δ = inv(prevfloat(typemax(nrm)))
if nrm ≥ δ # Safe to multiply with inverse
return map(Base.Fix2(*, inv(nrm)), a)
else # scale elements to avoid overflow
εδ = eps(one(nrm))/δ
result = map(Base.Fix2(*, εδ), a)
return rmul!(result, inv(nrm*εδ))
end
endThis duplicates the logic in __normalize!, but that seems easier than duplicating the logic in map?
Do you have an example of an array type where this fails? |
|
For example any GPU vector doesn't support it. The rest of the normalize function is supported. |
The previous implementation of
normalizewas computing the division between the first element of the array and the norm, in order to obtain the output type. This is both inefficient and doesn't work on arrays that don't support scalar indexing.I have changed the type check with
float(Base.promote_eltype(a, nrm)).