Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

[Clojure] Remove unneeded test files #14813

Merged

Conversation

gigasquid
Copy link
Member

@gigasquid gigasquid commented Apr 26, 2019

Description

Removes unneeded test files from #14800

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

@gigasquid
Copy link
Member Author

@Chouffe please take a look 👀

@Chouffe
Copy link
Contributor

Chouffe commented Apr 27, 2019

Should we add those two files in the .gitignore - similar to what we do for the following files:

test/test-ndarray.clj
test/test-ndarray-api.clj
test/test-symbol.clj
test/test-symbol-api.clj

@Chouffe
Copy link
Contributor

Chouffe commented Apr 27, 2019

Also, not sure how to deal with @kedarbellare great feedback

1. DRY the code a bit (to share as much as possible between symbol/ndarray api and random api)
2. call the scala SymbolRandomAPI and NDArrayRandomAPI
3. unify the random_ and sample_ methods (and remove them from the ndarray/symbol api namespaces)
4. add unit tests
1. Done
2. I could not get this to work when calling directly the `NDArrayRandomAPI` and the `SymbolRandomAPI`
3. This is not done yet. I am not entirely sure how to do that. Can it be done in a subsequent PR?
4. Done

Should we open issues for the things that were not done yet an tackle them in a subsequent PR? @gigasquid and @kedarbellare?

Also, I would like to see some actual unit tests for the generated random and sample functions in the 2 new namespaces. Let me know how we should proceed :)

@gigasquid
Copy link
Member Author

@Chouffe the adding the file to .gitignore is a great idea.

I'm totally cool with opening issues new PR to address the rest of the feedback from @kedarbellare as long as he is cool with it :)

@kedarbellare
Copy link
Contributor

+1 to adding the test-* files to .gitignore. i did the same with the ndarray-api files.

For the NDArrayRandomAPI part, it took some time but you can do something like:

(import '(org.apache.mxnet NDArrayOrScalar))

;; this calls random_exponential
(util/coerce-return
  (NDArrayRandomAPI/exponential
    (util/->option 1.0) ; lam
    (util/->option (mx-shape/->shape [2 1])) ; shape
    (util/->option nil) ; ctx
    (util/->option nil) ; dtype
    (util/->option nil) ; out
    (NDArrayOrScalar. true) ; input lambda is ndarray or scalar?
    nil ; class type
  ))
;; returns 2 x 1 randomly sampled ndarray

;; this calls sample_exponential (concurrent sampling)
(util/coerce-return
  (NDArrayRandomAPI/exponential
    (util/->option (ndarray/ones [3])) ; lam
    (util/->option (mx-shape/->shape [2 1])) ; shape
    (util/->option nil) ; ctx
    (util/->option nil) ; dtype
    (util/->option nil) ; out
    (NDArrayOrScalar. false) ; input lambda is ndarray or scalar?
    nil ; class type
  ))
;; returns 3 x 2 x 1 concurrently sampled ndarray

you can ignore my comment about unifying random/sample or come up with the clojure way of doing this (e.g. using defmulti and defmethod).

doing it in a subsequent PR is fine by me 😃

Copy link
Contributor

@kedarbellare kedarbellare left a comment

Choose a reason for hiding this comment

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

lgtm (adding it to .gitignore would be great!)

@Chouffe
Copy link
Contributor

Chouffe commented Apr 29, 2019

alright then! @gigasquid you can add the files in the .gitignore and merge. I will tackle @kedarbellare comments in subsequent PRs then!

@roywei
Copy link
Member

roywei commented Apr 29, 2019

@mxnet-label-bot add[Clojure, Test]

@gigasquid gigasquid force-pushed the remove-unneeded-clojure-test-files branch from b7f6cfb to 4ad5997 Compare May 1, 2019 15:11
@gigasquid gigasquid merged commit 1540a84 into apache:master May 2, 2019
@gigasquid gigasquid deleted the remove-unneeded-clojure-test-files branch May 2, 2019 18:39
access2rohit pushed a commit to access2rohit/incubator-mxnet that referenced this pull request May 14, 2019
* remove unneeded test files

* add test files to gitignore
haohuanw pushed a commit to haohuanw/incubator-mxnet that referenced this pull request Jun 23, 2019
* remove unneeded test files

* add test files to gitignore
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants