Skip to content
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

fixed bug and added test for generic vecnorm of array of arrays #12935

Merged
merged 1 commit into from
Sep 4, 2015

Conversation

stevengj
Copy link
Member

@stevengj stevengj commented Sep 3, 2015

As discussed in #12571, that PR inadvertently broke norm for arrays of arrays — norm should work on any iterable where a norm function is defined for the elements.

This fixes the bug add adds a test. It also speeds things up slightly for complex numbers, where abs2(abs(x)/one(x)) can be replaced by abs2(x).

@andreasnoack, I don't quite understand why you were dividing by one(x) (which is not defined for an arbitrary Banach space) in the first place. Was it to avoid overflow for squaring integers? That could be fixed by an appropriate call to float. Does this fix look okay to you?

@stevengj stevengj added regression Regression in behavior compared to a previous version linear algebra Linear algebra labels Sep 3, 2015
@stevengj stevengj added this to the 0.4.0 milestone Sep 3, 2015
@stevengj stevengj force-pushed the generic_vecnorm_test branch from d5c56bc to 95f3015 Compare September 3, 2015 21:15
@stevengj stevengj removed the regression Regression in behavior compared to a previous version label Sep 3, 2015
@stevengj
Copy link
Member Author

stevengj commented Sep 3, 2015

Whoops, not a regression with respect to 0.3, just with respect to previous 0.4-pre versions.

@stevengj stevengj added the bug Indicates an unexpected problem or unintended behavior label Sep 4, 2015
@andreasnoack
Copy link
Member

Yes. This could overflow for integers. You could argue that the promotion to floating point in vecnormInf is partly to blame here, because it complicates the check for overflow. Right now, the norm implementations are a bit floating-point-centric and not very generic with respect to the element type, but we have had this discussion before. I don't expect that you have changed your mind here, but probably, we will soon have an unum implementation and at some point FixedPointNumbers might also support sqrt and someone therefore might want to compute the norm of such vectors.

The division with one is added to help type inference bacause, alternatively, only one of the branches divides the summation terms. I checked the LLVM code, and in the most obvious cases there is never actually made a division in the loop. Since it's a constant it should be folded away, and it causes a promotion to a type there is more correct and inferred in an extensible/generic way that doesn't assume floating point elements, but unfortunately it assumes Number elements. I guess a fix could be to divide with one(norm(zero(x))), but I don't know if you want to follow that route.

@stevengj
Copy link
Member Author

stevengj commented Sep 4, 2015

@andreasnoack, since our current implementation already used promote_types(Float64, T), I don't think that the float call I inserted introduces changes the type of the result. It should also protect against overflow — do you have an example where you think the current implementation could overflow?

@andreasnoack
Copy link
Member

No. I think the float in the definition of norm_sqr in line 117 solves the overflow issue for integers.

andreasnoack added a commit that referenced this pull request Sep 4, 2015
fixed bug and added test for generic vecnorm of array of arrays
@andreasnoack andreasnoack merged commit a4a321a into JuliaLang:master Sep 4, 2015
@stevengj stevengj deleted the generic_vecnorm_test branch September 4, 2015 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants