Adds maxabs check in norm2

[ferrolho]

This PR addresses #963. I manually checked this and it does indeed get rid of the NaNs. The values of the jacobians in the MWE also agree with each other.

@mateuszbaran, could you please review these changes? I am not sure if the check should be done where I put it. Thank you!

[ferrolho]

Okay, I just found out that this solves the issue if norm2 is called explicitly, i.e., norm(x, 2), but the issue is still there if we call norm(x). I think that may be because I added the check in the wrong place...

[mateuszbaran]

I think the check should be in @generated function _norm(::Size{S}, a::StaticArray) where {S} which should cover all possible calls. The main issue is that this makes the method slower for the most common case of the array not being zero, and, after all, mathematically norm does not have a gradient at 0. StaticArrays usually picks speed over safety so I'd prefer to have input from other maintainers whether this change is desirable.

[ferrolho]

I understand that this will incur extra computation time, but I don't think it is correct to leave it as it is, since it is inconsistent with the numerical results from Julia base. Nonetheless, if you guys decide not to go forward with this change due to the extra computation time, I would like to propose an alternative safe_norm method where these checks would be done.

[thchr]

Could someone explain why this checks are supposedly missing or needed? From reading the linked issues, I understand that this somehow affects AD - but how the added checks involve this isn't clear.

As context, the analogous line of Base code linked in #908 (comment) appears to originate from JuliaLang/julia#10250 and was then, as I understand it, motivated by the fact that Base's norm rescales the input in a hypot-like manner (so, the check ensures that the rescaling approach is OK). There's no rescaling done in StaticArray's implementation (so, it is e.g. susceptible to overflow sooner): because of this, I am struggling to see the purpose of this additional check.

It seems this change won't affect ordinary output of norm(::StaticArray, 2) at all (it already works correctly for 0- and inf-entry input already) but just introduce additional branches?

[mateuszbaran]

I didn't see any reason to add this check apart from making ForwardDiff.jl work as expected but I didn't analyse it particularly deeply and assumed that maybe I'm missing something. From your investigation it just looks like a ForwardDiff.jl issue, unless we actually want that rescaling in StaticArrays.jl (we probably don't?)

[thchr]

The hypot-like rescaling would be nice in principle but would come at the cost of including floating-point divisions, which would presumably entail a performance hit (so probably not consistent with the philosophy of StaticArrays).

I agree it is not clear that this is a StaticArrays issue. As somewhat circumstantial evidence, #908 doesn't intersect with norm in any obvious way.

[ferrolho]

Thank you both for your comments. If this is not an issue with StaticArrays.jl, do you have any idea where the issue could be then? To be clear, when I found that AD-ing a function which takes the norm2 of a StaticArray type with zeros lead to a jacobian with NaNs, I was not sure if the problem was with ForwardDiff.jl or with StaticArrays.jl. But since the jacobian did not have NaNs for Base array types, and @KristofferC used git bisect to find #908 as the PR where this started happening, I decided to open #963 (where @mateuszbaran suggested adding the check in this PR).

[mcabbott]

I think that ForwardDiff is doing something like 0/0 here in the generic algorithm, but the special case (maxabs == 0 || isinf(maxabs)) && return maxabs picks a particular "sub-gradient" instead. That behaviour is itself a strange quirk we should fix, IMO, and not a good reason to add paths to StaticArrays.

[thchr]

I think we can probably close this PR, given the above discussion, correct? I'll close in a few days if there's no objections.

[ferrolho]

For the record, I have switched to using LinearAlgebra.generic_norm2 instead of norm(x, 2) in the parts of my application where this was an issue. As shown in JuliaRobotics/RigidBodyDynamics.jl#622, generic_norm2 solves the NaN problem in Jacobians computed with ForwardDiff.jl on StaticArray types. I think that may be the best thing to do, if someone runs into this issue in the future. As such, I do think we should close this PR. (I'll close it now.)