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

#13441 [Clojure] Add Spec Validations for the Random namespace #13523

Merged
merged 1 commit into from
Dec 6, 2018

Conversation

hellonico
Copy link
Contributor

@hellonico hellonico commented Dec 4, 2018

Description

[Clojure] Add Spec Validations for the Random namespace

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)

Changes

optimizer.clj: fix a previously added spec
random.clj: added clojure specs
operator_test.clj: expect ctx to be in a map
random_test.clj: update context key to ctx, added tests for random/normal, random/uniform and random/seed

@gigasquid
Copy link
Member

Thanks @hellonico ! I'll take a look shortly

@gigasquid gigasquid added Clojure pr-awaiting-review PR is waiting for code review labels Dec 4, 2018
@gigasquid
Copy link
Member

Thanks for making this better!

I ran some of the examples and found some bugs with the improved validation:

In examples/neural-style
Caused by: clojure.lang.ExceptionInfo: Incorrect adam optimizer options {:clojure.spec.alpha/problems ({:path [:learning-rate], :pred clojure.core/float?, :val 10, ...
and
Incorrect random uniform parameters #:clojure.spec.alpha{:problems [{:path [], :pred map?, :val #object[org.apache.mxnet.Context 0x752b69e3 "cpu(0)"], ...

Would you mind correcting the problems in the example as well?

Thanks 💯

@hellonico
Copy link
Contributor Author

having a look ...

@hellonico
Copy link
Contributor Author

@gigasquid

I see.

1/ The first one is due to the spec below:
The spec is defined as:

(s/def ::learning-rate float?)

while the received parameter for the optimizer is: (note the 10 instead of 10.0)

(opt/adam {:learning-rate 10
                             :wd 0.005
                             :lr-scheduler lr-sched})

If I make sure we cast to float all the different parameters, I can make the specs to be more flexible with:

(s/def ::int-or-float (s/or :f float? :i int?))

That works ?

2/ Second one is

(random/uniform 0 255 content-np-shape dev)

Where the dev should be passed inside the map (as I understand it) and which probably means the context was being ignored.

Is that ok to change the examples to use a map for the context ?

@gigasquid
Copy link
Member

It's definitely fine to change the example to make it work correctly :)

I'm ok with either changing the example code to be a float or we can think about loosening the spec to be a number?. What do you think would be more user friendly? I don't have strong feelings on it.

@hellonico
Copy link
Contributor Author

hellonico commented Dec 6, 2018

What do you think would be more user friendly?

number? was a better choice, and is nicer to the user.
I have updates the specs.

It's definitely fine to change the example to make it work correctly :)

I am glad you agreed ;)

@gigasquid
Copy link
Member

Thanks @hellonico for your work in improving this. I ran through the examples and everything looks good. It is good to go when CI is green.

@gigasquid gigasquid removed the pr-awaiting-review PR is waiting for code review label Dec 6, 2018
@gigasquid gigasquid merged commit 8feb826 into apache:master Dec 6, 2018
zhaoyao73 pushed a commit to zhaoyao73/incubator-mxnet that referenced this pull request Dec 13, 2018
zhaoyao73 added a commit to zhaoyao73/incubator-mxnet that referenced this pull request Dec 13, 2018
* upstream/master: (54 commits)
  Add notes about debug with libstdc++ symbols (apache#13533)
  add cpp example inception to nightly test (apache#13534)
  Fix exception handling api doc (apache#13519)
  fix link for gluon model zoo (apache#13583)
  ONNX import/export: Size (apache#13112)
  Update MXNetTutorialTemplate.ipynb (apache#13568)
  fix the situation where idx didn't align with rec (apache#13550)
  Fix use-before-assignment in convert_dot (apache#13511)
  License update  (apache#13565)
  Update version to v1.5.0 including clojure package (apache#13566)
  Fix flaky test test_random:test_randint_generator (apache#13498)
  Add workspace cleaning after job finished (apache#13490)
  Adding test for softmaxoutput (apache#13116)
  apache#13441 [Clojure] Add Spec Validations for the Random namespace (apache#13523)
  Revert "Bumped minor version from 1.4.0 to 1.5.0 on master, updated License file" (apache#13558)
  Chi_square_check for discrete distribution fix (apache#13543)
  Updated docs for randint operator (apache#13541)
  Simplifications and some fun stuff for the MNIST Gluon tutorial (apache#13094)
  Fix apache#13521 (apache#13537)
  Add a retry to qemu_provision (apache#13551)
  ...
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants