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

Port of scala infer package to clojure#13595

Merged
gigasquid merged 8 commits intoapache:masterfrom
kedarbellare:kedarb--clj-infer-package
Dec 27, 2018
Merged

Port of scala infer package to clojure#13595
gigasquid merged 8 commits intoapache:masterfrom
kedarbellare:kedarb--clj-infer-package

Conversation

@kedarbellare
Copy link
Contributor

@kedarbellare kedarbellare commented Dec 10, 2018 •
edited
Loading

Description

  • This PR contains the port of Scala inference package to Clojure (Issue [Clojure] - Port Scala Inference to Clojure Package #13384).
  • It ports the Predictor, Classifier, ImageClassifier and ObjectDetector APIs.
  • Docs, unit tests and examples still need to be added. I'll be doing these in the upcoming commits.

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • 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 http://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

  • Clojure infer package, tests, (and when applicable, API doc)

Reviewers

@gigasquid

@gigasquid
Copy link
Member

gigasquid commented Dec 10, 2018

@kedarbellare awesome! Thanks for bringing this to life.

@gigasquid gigasquid added Clojure pr-work-in-progress PR is still work in progress labels Dec 10, 2018
@kedarbellare
Copy link
Contributor Author

kedarbellare commented Dec 11, 2018

is there a recommended way to unit test this or should i just write integration tests?

@kedarbellare
Copy link
Contributor Author

kedarbellare commented Dec 17, 2018

@gigasquid this PR is mostly ready for review. PTAL when you get a chance. i'm not sure how to write unit tests for this though.

@gigasquid
Copy link
Member

gigasquid commented Dec 17, 2018

Wonderful 💯 - I'll take a look at it in the next few days. Thanks.

@kedarbellare kedarbellare force-pushed the kedarb--clj-infer-package branch 2 times, most recently from dbe39f6 to 493561d Compare December 18, 2018 05:11
gigasquid
gigasquid reviewed Dec 21, 2018
View reviewed changes
Copy link
Member

@gigasquid gigasquid left a comment

Choose a reason for hiding this comment

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

Thanks so much for tackling this. I gave it a test run and it works great. It is going to be a big win for clojure users.

The main thing that we need to figure out is how to incorporate some unit tests so we can make sure stuff doesn't break on PRs. I made a suggestion to adapt the example tests that you have. I don't think the hit of downloading some small models will be too much of an issue and the inference run time is pretty small.

I'm open to other suggestions as well.

Great job and ping me when the feedback is addressed for another pass.

contrib/clojure-package/src/org/apache/clojure_mxnet/infer.clj
@@ -0,0 +1,210 @@
;;
Copy link
Member

@gigasquid gigasquid Dec 21, 2018

Choose a reason for hiding this comment

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

Nicely done!

@kedarbellare kedarbellare force-pushed the kedarb--clj-infer-package branch from 493561d to a15a098 Compare December 22, 2018 02:03
@kedarbellare
Copy link
Contributor Author

kedarbellare commented Dec 22, 2018

@gigasquid : almost all the feedback is addressed. adding specs is remaining. PTAL

@gigasquid
Copy link
Member

gigasquid commented Dec 22, 2018

Thanks for addressing the feedback so quickly!
I'll take another pass through it shortly and give it a test drive. Looking good :)

@gigasquid gigasquid merged commit b6b197a into apache:master Dec 27, 2018
@azai91 azai91 mentioned this pull request Dec 29, 2018
Open
rondogency pushed a commit to rondogency/incubator-mxnet that referenced this pull request Jan 9, 2019
@kedarbellare
Port of scala infer package to clojure (apache#13595)
e66dbfc 
* Port of scala infer package to clojure

* Add inference examples

* Fix project.clj

* Update code for integration tests

* Address comments and add unit tests

* Add specs and simplify interface

* Minor nit

* Update README
haohuanw pushed a commit to haohuanw/incubator-mxnet that referenced this pull request Jun 23, 2019
@kedarbellare
Port of scala infer package to clojure (apache#13595)
158c67a 
* Port of scala infer package to clojure

* Add inference examples

* Fix project.clj

* Update code for integration tests

* Address comments and add unit tests

* Add specs and simplify interface

* Minor nit

* Update README
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@gigasquid gigasquid gigasquid left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Clojure pr-work-in-progress PR is still work in progress

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@kedarbellare @gigasquid