[MXNET-913] Java API --- Scala NDArray Improvement#12536
[MXNET-913] Java API --- Scala NDArray Improvement#12536lanking520 wants to merge 9 commits intoapache:masterfrom
Conversation
|
Thanks for your contribution @lanking520 |
|
@lanking520 I really like to toString changes to improve readability but I wonder about the behavior as the matrices get large. Do we want to print out 1000x1000 arrays by default? |
e003681 to
26cf6b9
Compare
| ) | ||
| val output = List( | ||
| ("org.apache.mxnet.Symbol", true), | ||
| ("Int", false), |
There was a problem hiding this comment.
wouldn't this break backward compatibility?
There was a problem hiding this comment.
This test is just for Macros generation. As long as integration test passed, it didn't break the BC.
| * This arg Builder is intent to solve Java to Scala conversion | ||
| * to take the input such as (arg: Any*) | ||
| */ | ||
| class ArgBuilder { |
There was a problem hiding this comment.
why do we need this? why are you going back in time? we want to move away from (arg: Any*) to type-safe APIs
There was a problem hiding this comment.
I think type-safe is not as important as ease of usage. For type-safe in Java, the major question is how to deal with default args.
There was a problem hiding this comment.
I guess Java does not get defaults. @andrewfayres rightly asked how many APIs do we have that have a large number of parameters to matter? may be we just add builder to those if they are only a few and for others they can pass Scala Option.None or change the APIs to accept gauva Optional.
@lanking520 can you find how many have more than 5 parameters?
There was a problem hiding this comment.
After running this:
printf(s"\n\n\n\nTotal numbers " +
ndarrayFunctions.count(_.listOfArgs.length > 5)
+ " out of " + ndarrayFunctions.length + "\n\n\n\n"
)get output
Total numbers 64 out of 665
However, we should consider out as a param in Type-safe API param. Counting this we got
Total numbers 101 out of 665
There was a problem hiding this comment.
It's not the total number of parameters that matters, it's how many default parameters there are in the method. If a method has 5 parameters and no defaults then the builder doesn't help any. If there are 5 parameters and all are defaults then it helps a lot.
@lanking520 Can we get a count of how many methods have more than 3 default args? If possible what I'd really like is a distribution (x methods have 1 default arg, y have 2, ...) but if this is too difficult I understand.
There was a problem hiding this comment.
printf(s"\n\n\n\nTotal numbers " +
ndarrayFunctions.count(_.listOfArgs.count(_.isOptional) > 3)
+ " out of " + ndarrayFunctions.length + "\n\n\n\n"
)
Here you go
Total numbers 65 out of 665
There was a problem hiding this comment.
So about 10% of NDArray methods have more than 3 default args.
I'm going to give some more thought before I commit to this but my initial reaction is that we include this but try not to promote it's use too much in docs/examples. Leave it there as an ease of use option for the customer with the understanding that when they use this they will be giving up type-safety.
|
@gigasquid in order to solve this issue. I have done two things:
|
|
@lanking520 may be just printing the NDArray metadata in toString might be suffient
|
Hi @nswamy , the point for The reason I set a limitation there is because I am facing this issue while I am trying to get a |
| * @return A copy of array content. | ||
| */ | ||
| def toArray: Array[Float] = { | ||
| require(shape.toArray.product < 1000000, |
There was a problem hiding this comment.
we shouldn't add arbitrary limitations.
| } | ||
|
|
||
| override def toString: String = { | ||
| buildStringHelper(this, this.shape.length) + "\n" |
There was a problem hiding this comment.
print NDArray's metadata instead
(x.shape, x.size, x.dtype, x.context, x.handle(%x)->hex value of native mem ref)
436d46b to
687d0f7
Compare
|
@lanking520 Could you check the build failure |
93647e1 to
bf8792d
Compare
|
@lanking520 Requesting an update on the PR, is the build failure issue resolved? |
|
Stop the experiment and ship with this solution #12772 |
Description
This PR contains some addition in NDArray as well as Java compatible functionalities for Scala package.
ArgBuilder: Allows user to pass in a single variable or a batch variables to construct a Scala Sequence or Scala Map.NDArray:toStringmethod that can allow user to see a formatted output.new NDArray()constructor allowing Java users to create a new one through this way.add,subtractmethods to allow java user use themUnit test is coming soon...
@nswamy @yzhliu @andrewfayres
Demo code
Output:
Checklist
Essentials
Please feel free to remove inapplicable items for your PR.