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

[hellonico]

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]

Thanks @hellonico ! I'll take a look shortly

[gigasquid]

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]

having a look ...

[hellonico]

@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]

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]

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]

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.