-
Notifications
You must be signed in to change notification settings - Fork 6.8k
[MXNET-357] New Scala API Design (NDArray) #10787
Conversation
NDArray one is in Sync with #10660 |
@@ -102,20 +178,80 @@ private[mxnet] object NDArrayMacro { | |||
result | |||
} | |||
|
|||
|
|||
// Convert C++ Types to Scala Types | |||
private def typeConversion(in : String, argType : String = "") : String = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we have these functions shared with those in SymbolMacro?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's change it in Document integration component PR, we can do a global migration on Symbol, NDArray there. Any recommendation where we put these functions? As a single file or inside SymbolMacros.
44d93e0
to
441937c
Compare
var argDef = ListBuffer[String]() | ||
ndarrayfunction.listOfArgs.foreach(ndarrayarg => { | ||
val currArgName = ndarrayarg.argName match { | ||
case "var" => "vari" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please add comments why you are transforming these.
}) | ||
argDef += "name : String = null" | ||
argDef += "attr : Map[String, String] = null" | ||
// Construct Implementation field |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you expand the comment on why you are doing it here again
ie., you are still taking all the parameters and sending that across the JNI boundary as a map otherwise it would need to rewrite all JNI API implementation to match these. There is a small problem with this approach, we are not tying the type the backend expects from the Scala API. ie., we might by mistake generate a different type(if the mapping below is wrong)
441937c
to
9fd6505
Compare
Add 1 line change in Sync with this PR: #11021 |
* Add new NDArray APIs * Add NDArray APIs * change the impl into individual functions and add comments * Quick fix on redudant code * Change in Sync
* Add new NDArray APIs * Add NDArray APIs * change the impl into individual functions and add comments * Quick fix on redudant code * Change in Sync
* Add new NDArray APIs * Add NDArray APIs * change the impl into individual functions and add comments * Quick fix on redudant code * Change in Sync
Description
See full design document
@nswamy @yzhliu
This PR is the Addition for new NDArray functions of Scala API
Warning: current PR expose underscore functions
Checklist
Essentials
Please feel free to remove inapplicable items for your PR.