pishen commented on a change in pull request #9771: Modify NDArrayIter 
constructor to receive tuple (i.e. dict in Python)?
URL: https://github.com/apache/incubator-mxnet/pull/9771#discussion_r167953138
 
 

 ##########
 File path: scala-package/core/src/main/scala/ml/dmlc/mxnet/io/NDArrayIter.scala
 ##########
 @@ -38,15 +38,14 @@ import scala.collection.immutable.ListMap
  * the size of data does not match batch_size. Roll over is intended
  * for training and can cause problems if used for prediction.
  */
-class NDArrayIter (data: IndexedSeq[NDArray], label: IndexedSeq[NDArray] = 
IndexedSeq.empty,
-                  private val dataBatchSize: Int = 1, shuffle: Boolean = false,
-                  lastBatchHandle: String = "pad",
-                  dataName: String = "data", labelName: String = "label") 
extends DataIter {
+class NDArrayIter (data: IndexedSeq[(String, NDArray)], label: 
IndexedSeq[(String, NDArray)],
+                  private val dataBatchSize: Int, shuffle: Boolean,
+                  lastBatchHandle: String) extends DataIter {
 
 Review comment:
   It said:
   
   > In Scala, every auxiliary constructor must invoke another constructor of 
the same class as its first action. In other words, the first statement in 
every auxiliary constructor in every Scala class will have the form "this(...)".
   
   So my word was not precise, it can also call other constructors, but still 
not other functions.
   
   I think this PR already tried to structure the primary constructor as a 
"common" protion, and use the `def this()` at [line 
77](https://github.com/parallelgithub/incubator-mxnet/blob/3bdb52c25b959fd34fac146fe7d2d818b8be6eec/scala-package/core/src/main/scala/ml/dmlc/mxnet/io/NDArrayIter.scala#L77)
 to translate the old constructor's inputs into the primary constructor to 
support backward compatibility?
   
   I'm not sure if this will cause any error for the users using current 
version, though. (At least the test passed :P) Maybe we can examine the test 
(or add some more tests) to show that it still works in the original use case?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

Reply via email to