[Clojure] Helper function for n-dim vector to ndarray #14305

kedarbellare · 2019-03-03T01:24:09Z

Description

Adds helper function to convert n-dimensional vector to NDArray. It can be used for any n-dimensional float/double vector as long as it meets the requirements of a ndarray.
This addresses the issue [Clojure] Add helper function to convert formed vector to NDArray (infers shape) #14271

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
Changes are complete (i.e. I finished coding on this PR)
All changes have test coverage:
Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
Code is well-documented:
For user-facing API changes, API doc string has been updated.
For new C++ functions in header files, their functionalities and arguments are documented.
For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
Check the API doc at https://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

->ndarray with tests and docs

gigasquid · 2019-03-03T13:20:19Z

Thanks. This looks super 👍 - I'll check it out in closer detail shortly, (and find out why the CI seems stuck)

Chouffe

do we want to also think about a ->nd-vec function that would do the opposite operation? Taking an NDArray and returning a Clojure nd-vec?
Here is a proposal: Chouffe@282d863

Chouffe · 2019-03-03T17:28:00Z

contrib/clojure-package/src/org/apache/clojure_mxnet/ndarray.clj

+(defn ->ndarray
+ "Creates a new NDArray based on the given n-dimensional
+ float/double vector.
+ `nd-vec`: n-dimensional vector with floats or doubles.


Do we want to allow also integers here? or other numerical types that can be understood by MXNet?

Maybe the Scala functions will not allow it but we can cast to float/double from clojure?

currently, scala doesn't allow numerical types other than float/double.

usually i've been using ndarray/as-type for type conversion instead of doing conversion within clojure. my hunch is as-type is faster but i've not verified this. however, one disadvantage is that it returns a copy of the array in the new type.

From a user perspective, I think it would be nice to be able to handle ((ndarray/->ndarray [5 -4 3]) and not throw an exception. What do you think about adding a check in the util function to see if any element is an integer and if so, convert it to double. Ex:

(if (some int? s) (to-array (mapv double s)) (to-array s))

Chouffe · 2019-03-03T17:28:14Z

contrib/clojure-package/src/org/apache/clojure_mxnet/ndarray.clj

+ `ctx`: Context of the output ndarray, will use default context if unspecified.
+ }
+ returns: `ndarray` with the given values and matching the shape of the input vector.
+ Ex:


Love your docstring!

Chouffe · 2019-03-03T17:30:09Z

contrib/clojure-package/src/org/apache/clojure_mxnet/util.clj

+ [s]
+ (validate! ::non-empty-seq s "Invalid N-D sequence")
+ (if (sequential? (first s))
+ (to-array (map to-array-nd s))


Should we call mapv? So that thunks for the lazy sequence are not built up?

anirudhacharya · 2019-03-04T00:33:34Z

@mxnet-label-bot add [pr-awaiting-review, Clojure]

kedarbellare · 2019-03-04T01:06:08Z

@Chouffe : having an ->nd-vec could be potentially useful although the recent (str ndarray) is much clearer for debugging (#14272). do you want to submit a PR for ->nd-vec?

Chouffe · 2019-03-04T08:10:10Z

@kedarbellare here is the PR - WIP I need to add the unit tests
#14308
I'd love your feedback

gigasquid · 2019-03-08T16:00:07Z

taking a look 👀

gigasquid

Thanks so much for your work on this @kedarbellare. Overall looks great, just a comment on how we can support the user friendly use case of a user putting in int values.

gigasquid · 2019-03-08T18:18:33Z

contrib/clojure-package/src/org/apache/clojure_mxnet/util.clj

+ "Converts any N-D sequential structure to an array
+ with the same dimensions."
+ [s]
+ (validate! ::non-empty-seq s "Invalid N-D sequence")


Thanks for putting this validation in

gigasquid · 2019-03-08T19:35:55Z

contrib/clojure-package/src/org/apache/clojure_mxnet/ndarray.clj

+(defn ->ndarray
+ "Creates a new NDArray based on the given n-dimensional
+ float/double vector.
+ `nd-vec`: n-dimensional vector with floats or doubles.


From a user perspective, I think it would be nice to be able to handle ((ndarray/->ndarray [5 -4 3]) and not throw an exception. What do you think about adding a check in the util function to see if any element is an integer and if so, convert it to double. Ex:

(if (some int? s) (to-array (mapv double s)) (to-array s))

gigasquid · 2019-03-08T19:42:32Z

contrib/clojure-package/src/org/apache/clojure_mxnet/util.clj

+ [s]
+ (validate! ::non-empty-seq s "Invalid N-D sequence")
+ (if (sequential? (first s))
+ (to-array (mapv to-array-nd s))


recursion for the win! 🥇

kedarbellare · 2019-03-09T17:35:33Z

@gigasquid @Chouffe found a better way! PTAL 😃

gigasquid

Looks great. Will be good to go after CI turns green

gigasquid · 2019-03-09T20:49:08Z

contrib/clojure-package/src/org/apache/clojure_mxnet/util.clj

+ (to-array (mapv to-array-nd nd-seq))
+ (to-array nd-seq)))
+
+(defn nd-seq-shape


* [Clojure] Helper function for n-dim vector to ndarray * More tests, specs and rename method * Address comments * Allow every number type

[Clojure] Helper function for n-dim vector to ndarray

c98031c

kedarbellare requested a review from gigasquid as a code owner March 3, 2019 01:24

More tests, specs and rename method

5a90966

Chouffe reviewed Mar 3, 2019

View reviewed changes

marcoabreu added Clojure pr-awaiting-review PR is waiting for code review labels Mar 4, 2019

Address comments

a26dafe

gigasquid reviewed Mar 8, 2019

View reviewed changes

Allow every number type

3aaaa8e

gigasquid approved these changes Mar 10, 2019

View reviewed changes

gigasquid removed the pr-awaiting-review PR is waiting for code review label Mar 10, 2019

gigasquid merged commit a4b9802 into apache:master Mar 11, 2019

nswamy pushed a commit that referenced this pull request Apr 5, 2019

[Clojure] Helper function for n-dim vector to ndarray (#14305)

f981f4e

* [Clojure] Helper function for n-dim vector to ndarray * More tests, specs and rename method * Address comments * Allow every number type

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Clojure] Helper function for n-dim vector to ndarray #14305

[Clojure] Helper function for n-dim vector to ndarray #14305

kedarbellare commented Mar 3, 2019

gigasquid commented Mar 3, 2019

Chouffe left a comment •

edited

Loading

Chouffe Mar 3, 2019

Chouffe Mar 3, 2019

kedarbellare Mar 4, 2019

kedarbellare Mar 4, 2019 •

edited

Loading

gigasquid Mar 8, 2019

Chouffe Mar 3, 2019

Chouffe Mar 3, 2019

kedarbellare Mar 4, 2019

anirudhacharya commented Mar 4, 2019

kedarbellare commented Mar 4, 2019

Chouffe commented Mar 4, 2019

gigasquid commented Mar 8, 2019

gigasquid left a comment

gigasquid Mar 8, 2019

gigasquid Mar 8, 2019

gigasquid Mar 8, 2019

kedarbellare commented Mar 9, 2019

gigasquid left a comment

gigasquid Mar 9, 2019

[Clojure] Helper function for n-dim vector to ndarray #14305

[Clojure] Helper function for n-dim vector to ndarray #14305

Conversation

kedarbellare commented Mar 3, 2019

Description

Checklist

Essentials

Changes

gigasquid commented Mar 3, 2019

Chouffe left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kedarbellare Mar 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anirudhacharya commented Mar 4, 2019

kedarbellare commented Mar 4, 2019

Chouffe commented Mar 4, 2019

gigasquid commented Mar 8, 2019

gigasquid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kedarbellare commented Mar 9, 2019

gigasquid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chouffe left a comment •

edited

Loading

kedarbellare Mar 4, 2019 •

edited

Loading