Handling for arrays of dimesionality 0 (scalar) and 1 (vector)

[mikera]

Thanks for a great start on a ND4J librray for Clojure!

Having had a play with the code, I think this biggest immediate issue is how to handle arrays with diminsionality 0 and 1. ND4J seems to assume that everything has a rank of 2 or above. This seems to be a design flaw on the part of ND4J, to fix it we need to ensure that shape and dimensionality are correct in the Clojure side.

Example:

(.shape (array :nd4j [1 2]))
=> [1, 2]

In core.matrix terms, the shape should be simply [2], i.e. with a dimensionality of 1

I've fixed dimensionality and shape in my branch (https://github.com/mikera/nd4clj) however there are still a lot of other errors in the compliance tests after these changes - most probably because the other functions aren't handling shape correctly. Let me know if you want a PR

[ds923y]

The only way I have thought to fix this is to provide an adapter for NDArray that will support the dimensionality's 0 and 1.

The alternative and easiest fix is to provide more guards in the test cases like
(when (supports-dimensionality? im 1)
that make use of
(mp/supports-dimensionality? [m dimensions] (>= dimensions 2)).

The developers of nd4j are active on gitter.im they believe that supporting matrices below rank 2 is a testing burden at this time because of "to many edge cases". So not only does Nd4j assume matrices are of rank 2 according to them matrices below rank 2 are not supported. To see if the fixes you provided will help I would have to look at their function implementations e.g.

    @Override
    public boolean isScalar() {
        if(isScalar != null)
            return isScalar;
        if (Shape.rank(shapeInformation) > 2) {
            isScalar = false;
        }
        else if (Shape.rank(shapeInformation) == 1) {
            isScalar = Shape.shapeOf(shapeInformation).getInt(0) == 1;
        }
        else if (Shape.rank(shapeInformation) == 2) {
            isScalar = Shape.shapeOf(shapeInformation).getInt(0) == 1
                    && Shape.shapeOf(shapeInformation).getInt(1) == 1;
        }

        else
            isScalar = false;

        return isScalar;
    }

to see if the code

  (mp/get-shape [m]
    (let [rank (.rank m)] 
      ;; we need to do this since NMD4J considers everything to have rank 2 or more
      (if (== rank 2)
        (cond 
          (.isScalar m) []
          (.isVector m) [(.length m)]) 
        (vec (.shape m)))))

would actually work.

You said "however there are still a lot of other errors in the compliance tests after these changes - most probably because the other functions aren't handling shape correctly. "
What kind of fix are suggesting?
Perhaps I can make a clojure type with deftype instead.
A pull request would not work at the moment because I forgot to do git rm src/clojure/nd4clj/kiw.clj
all of my implementations are in matrix.clj. So what I would need to do is to do git blame and see what changes you made and put the common sense ones in. When there is a plan to fix this either by having the compliance tests honor supports-dimensionality, making an adapter for the BaseNDArray objects, or by figuring out in what it means for dimensionality's less than 2 to not be supported in NDArray.

[mikera]

A wrapper type is possible, but I don't think it is strictly necessary. It will also add quite a bit of extra complexity.

I think the best way to handle this is by special cases in the protocol implementations, that should check isVector as isScalar as appropriate. My code for get-shape and dimensionality are offered as examples of how to do this. This is possibly going to be a little bit painful, but I don't think it can be avoided given that ND4J doesn't support dimenionality 0 or 1 properly by itself.

One other option is to use another implementation for 0D and 1D cases (e.g. Clojure vectors, or vectorz-clj types). I don't really like this idea though: mixing implementations is tricky and will introduce just more edge cases.

We can certainly patch the compliance tests to check supports-dimensionality in more places, but either way the code that implements the core.matrix protocols needs to return arrays with the correct dimensionality (whether that is an ND4J type or something else). The "default" implementations is core.matrix generally do the right thing (albeit with some performance cost), so it would be possible to just comment out protocol implementations until we have made them properly dimensionality aware.

[ds923y]

An adapter will open up the possibility for a pull request for nd4j. I will have faith isScalar et al. do the right thing and make and try to make the test cases work. I have made use of the default implementations before.

Thanks for the comments.

[ds923y]

For the test in file compliance_tester.cljc line 517 the shape implementation that you suggested causes some 2D implementations to promote down in dimension seemingly erroniously and therefore fail tests.

x==?	(fn [x] (vec (.shape x)))	(fn [x] (mat/shape x))
m1	[1 4]	[4]
m2	[4 3]	nil
m1xm2	[1 3]	[3]

expected: (mat/equals (mat/shape (mat/mmul m1 m2)) [1 3])
actual: (not (mat/equals [3] [1 3]))

Can you propose a path forward?

[ds923y]

fixing mat/shape to get rid of the nil will not help

(mp/get-shape [m]
    (let [rank (.rank m)] 
      ;; we need to do this since NMD4J considers everything to have rank 2 or more
      (if (== rank 2)
        (cond 
          (.isScalar m) []
          (.isVector m) [(.length m)]
          :else (vec (.shape m))) 
        (vec (.shape m)))))

produces

x==?	(fn [x] (vec (.shape x)))	(fn [x] (mat/shape x))
m1	[1 4]	[4]
m2	[4 3]	[4 3]
m1xm2	[1 3]	[3]

[ds923y]

I am working on a fix in the deftype_version branch

[ds923y]

The deftype_version has been merged with master. The issues at present with the compliance tests don't look to have much todo with 0d 1d matrices specifically, but I will wait a while to close.

[ds923y]

There are 0 tests failing now.