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

WIP. Norm cleanup. #5545

Merged
merged 1 commit into from
Feb 1, 2014
Merged

WIP. Norm cleanup. #5545

merged 1 commit into from
Feb 1, 2014

Conversation

andreasnoack
Copy link
Member

Devectorize norm calculation and avoid code dublication. Remove negative infinity norm because it violates the triangle inequality.

@jiahao
Copy link
Member

jiahao commented Jan 26, 2014

I actually like having p-norms that are not true vector norms. There are actually genuine use cases for p<1 norms. In addition to the p=-Inf "norm", the p=0 "norm", for example, counts the number of nonzero entries and shows up in compressed sensing problems. Why not just leave them in with the documented caveat that only p>=1 are true vector norms, since there is no actual numerical problem in computing them?

@tknopp
Copy link
Contributor

tknopp commented Jan 26, 2014

@jiahao +1 We all know that these are no "true" norms but in the CS field they are used a lot. This is programming. Every true mathematician has serious problems with statements like x=x+1

@andreasnoack
Copy link
Member Author

No problem. I had to introduce some promotion for handling negative p and thought it made more sense that norm was a norm, but if it useful, I am complete fine with extending the range of p to include non-norm norms.

I have updated the pull request with the changes.

return dx * (sum(absx.^p).^(1/p))
end
n == 0 && return zero(T)
p == 1 && return BLAS.asum(n, x, 1)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be BLAS.asum(n, x, stride(x, 1)). As it now accepts strided vector, the stride may not always be 1.
This alsp applies to BLAS.nrm2 below.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A test case should be added to ensure it works for sub(x, 1:2:n).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lindahua Thanks. I forgot the BLAS allows that.

I'll add some tests for the norms. Actually there are no specific tests for norm at all. Much testing is still needed for SubArrays in the BLAS and LAPACK code as the stride is not always calculated correct. I guess it hasn't been a problem because sub has been so slow, but this will probably change when people start to use fast views.

@andreasnoack
Copy link
Member Author

Please ignore the QR commits. They have their own PR. I had to rebase on that branch to get the new promotion rules I made there.

I have added tests for the norm function and it revealed that the 1 norm was not a norm for BlasComplex. I wrongly thought the documentation string "DZASUM takes the sum of the absolute values" meant that the function took the sum of the absolute values. What is actually calculated is the sum of the absolute values of the real and the imaginary part.

@andreasnoack
Copy link
Member Author

This one is ready for review. @StefanKarpinski I have tried to measure the cost of using invoke and I couldn't see a difference.

@jiahao
Copy link
Member

jiahao commented Jan 31, 2014

So DZASUM calculates abs(re+im) instead of abs(re)+abs(im)?

@andreasnoack
Copy link
Member Author

No DZASUM calculates norm(real(z),1)+norm(imag(z),1) and I thought it calculated norm(z,1).

@jiahao
Copy link
Member

jiahao commented Jan 31, 2014

So they would differ for a vector z. I see.

@jiahao
Copy link
Member

jiahao commented Jan 31, 2014

Everything else looks good.

@jiahao
Copy link
Member

jiahao commented Feb 1, 2014

looks ready to merge.

andreasnoack added a commit that referenced this pull request Feb 1, 2014
@andreasnoack andreasnoack merged commit cb26338 into master Feb 1, 2014
@andreasnoack andreasnoack deleted the anj/norm branch February 1, 2014 14:56
@jiahao
Copy link
Member

jiahao commented Feb 1, 2014

Code review on an empty stomach is clearly not my specialty.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants