-
Notifications
You must be signed in to change notification settings - Fork 6.8k
[clojure-package] add ->nd-vec
function in ndarray.clj
#14308
[clojure-package] add ->nd-vec
function in ndarray.clj
#14308
Conversation
@mxnet-label-bot add [pr-work-in-progress, Clojure] |
I would love some feedback on this @kedarbellare @gigasquid :-) |
taking a look 👀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Chouffe I think it looks really good 👍
Just a few minor comments, but I think it is a great direction and with some tests would be ready to merge.
Thanks for making this come to life!
(if-not (seq ss) | ||
(vec v) | ||
(->> v | ||
(partition (/ (count v) s1)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You will need to use clojure.core//
instead of just /
because the ndarray has a definition for the operator in this namespace
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch!
[org.apache.clojure-mxnet.context :as mx-context] | ||
[org.apache.clojure-mxnet.shape :as mx-shape] | ||
[org.apache.clojure-mxnet.util :as util] | ||
[clojure.reflect :as r] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mind cleaning up `[clojure.reflect ] while you are in here? It should be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
Ex: | ||
(->nd-vec (array [1] [1 1 1])) ;[[[1.0]]] | ||
(->nd-vec (ndarray/array [1 2 3] [3 1 1])) ;[[[1.0]] [[2.0]] [[3.0]]]" | ||
[ndarray] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the function spec, do you think it would also be nice to be able to do a util/validate
in here to check that it is a NDArray? when called without instrumentation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes lets do that!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mostly lgtm. adding tests would be great!
(s/def ::ndarray #(instance? NDArray %)) | ||
(s/def ::vector vector?) | ||
(s/def ::shape-vec-match-vec | ||
(fn [[v vec-shape]] (= (count v) (reduce * 1 vec-shape)))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
* WIP * Unit tests need to be added
Unit tests added and all checks have passed! |
->nd-vec
function in ndarray.clj
->nd-vec
function in ndarray.clj
Thanks for your contribution @Chouffe 😸 |
…he#14308) * [clojure-package][wip] add `->nd-vec` function in `ndarray.clj` * WIP * Unit tests need to be added * [clojure-package][ndarray] add unit tests for `->nd-vec` util fn
* [clojure-package][wip] add `->nd-vec` function in `ndarray.clj` * WIP * Unit tests need to be added * [clojure-package][ndarray] add unit tests for `->nd-vec` util fn
…he#14308) * [clojure-package][wip] add `->nd-vec` function in `ndarray.clj` * WIP * Unit tests need to be added * [clojure-package][ndarray] add unit tests for `->nd-vec` util fn
Description
Proposal for adding a
->nd-vec
function inndarray.clj
Checklist
Essentials
Please feel free to remove inapplicable items for your PR.