search

mikera / vectorz-clj

Fast matrix and vector maths library for Clojure - as a core.matrix implementation
203 stars 19 forks source link

extend PNorm and PMatrixRank protocols #34

Closed prasant94 closed 10 years ago

prasant94 commented 10 years ago

The build should pass as soon as there is a new vectorz release.

mikera commented 10 years ago

All looks sensible to me. Will do the new vectorz release now

mikera commented 10 years ago

Hmmm you need the element sum stuff for this right? So that should be in the next release 0.38.0

mikera commented 10 years ago

Can do this later today if you can make sure the Vectorz changes are fully tested etc.

prasant94 commented 10 years ago

Okay, I'll let you know when I've done that

prasant94 commented 10 years ago

I've just sent a new PR with more tests for norm. Hopefully, we should be good to release vectorz after that.

mikera commented 10 years ago

OK great. You want any stuff in core.matrix itself by the way? Going to do a core.matrix release today as well

prasant94 commented 10 years ago

Oh I somehow missed this message. No, nothing in core.matrix. Just ping me when you do the vectorz release. Thanks!

mikera commented 10 years ago

OK new vectorz release should be available now

prasant94 commented 10 years ago

This should now be good to merge. I slightly changed the PNorm function to handle the infinity norm case.

mikera commented 10 years ago

This line looks suspicious to me:

   :else (Math/pow (.elementAbsPowSum m 2) (/ 1 2))))

Seems to do the 2-norm on any other input - why is this? Shouldn't it throw an error if it gets passed a string like "Hedgehog"?

prasant94 commented 10 years ago

I thought we should perform the default operation when p has an unexpected value.

Do we need to have another case for (norm M :frobenius)? Won't (norm M 2) or just (norm M) cover that?

mikera commented 10 years ago

I prefer errors when we get an unexpected values! Otherwise it is likely to result in undetected bugs in user code.

I don't see any need to support a :frobenius keyword when 2 will do the trick.

prasant94 commented 10 years ago

This is the norm function in the current release. It has :frobenius has the default value of p. The protocol was probably updated after the release.

(defn norm
  "Computes norm of matrix or vector X. p argument specifies p norm.
   By default calculates 2 norm for vector and Frobenius norm for matrix.

   Special cases of p argument:
   Double/POSITIVE_INFINITY - Infinity norm
   :frobenius - Frobenius norm

   Intended usage: (let [n (norm v 1)] ....)
                   (let [n (norm v Double/POSITIVE_INFINITY)] ....)
                   (let [n (norm v)] ....)"
  ([m]
     (cond
      (vec? m) (norm m 2)
      (matrix? m) (norm m :frobenius)))
  ([m p]
     (cond
      (vec? m)(mp/vector-norm m p)
      (matrix? m) (mp/matrix-norm m p))))

So this will have to wait till the next core.matrix release is out.

mikera commented 10 years ago

Right so I think worth updating core.matrix to use 2 instead of :frobenius?