fix zeros in hypot and norm #162
Conversation
ext/AccessorsLinearAlgebraExt.jl
Outdated
| function set(arr, ::typeof(norm), val) | ||
| omul = iszero(val) ? one(norm(arr)) : norm(arr) | ||
| map(Base.Fix2(*, val / omul), arr) | ||
| end |
There was a problem hiding this comment.
oneunit instead? Or should we error? This seems to break lens laws.
There was a problem hiding this comment.
What laws does it break?
Setting norm(x) = 0 should always work, including when norm(x) is already zero.
Changed to oneunit, that's correct indeed!
There was a problem hiding this comment.
norm(set([0,0], norm, val)) == val will not hold for nonzero val. So maybe we should error if iszero(norm(arr))? Personally I want to get an error if I try to normalize the zero vector.
OTOH I suspect we have little violations like this in other places as well though. What do you think?
There was a problem hiding this comment.
Oh, that point is unrelated to this PR: here, we are only changing what happens for @set norm(...) = 0. Meanwhile, zero vector + non-zero norm scenario is already present in current Accessors:
julia> set([0,0], norm, 1)
2-element Vector{Float64}:
NaN
NaNThere definitely other cases of similar inequalities happening, eg:
julia> angle(set(0, angle, 0.5))
0.0
julia> o = @o filter(isodd, _)
julia> o(set([1,2,3], o, [4,6]))
Int64[]Btw, this works and conceptually similar to the norm issue:
julia> set(0+0im, abs, 0.5)
0.5 + 0.0imbut is specific to complex values and their sign() being defined even at zero.
We may think about putting a higher priority on strictly following optics laws and not defining "unlawful" optics. But we should also be careful to keep the Accessors flexibility: for example filter() optic above is clearly useful even in cases that break that law. Setting norm of a zero vector is at a boundary imo: maybe throwing an exception is good enough... At least it returns invalid values (NaNs) and not zeros :)
There was a problem hiding this comment.
@jw3126 is the PR fine in the current state? Zero norm input and similar can be discussed separately, here we don't change them at all.
No description provided.