[GitHub] marcoabreu commented on issue #9771: Modify NDArrayIter constructor to receive tuple (i.e. dict in Python)?

[GitBox]

marcoabreu commented on issue #9771: Modify NDArrayIter constructor to receive 
tuple (i.e. dict in Python)?
URL: https://github.com/apache/incubator-mxnet/pull/9771#issuecomment-365535322
 
 
   @nswamy do you have an idea?


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

[GitHub] marcoabreu commented on issue #9771: Modify NDArrayIter constructor to receive tuple (i.e. dict in Python)?

[GitBox]

marcoabreu commented on issue #9771: Modify NDArrayIter constructor to receive 
tuple (i.e. dict in Python)?
URL: https://github.com/apache/incubator-mxnet/pull/9771#issuecomment-365223393
 
 
   Your change has the following issues, not providing backwards compatibility:
   ```
   Type change: data: IndexedSeq[NDArray] -> data: IndexedSeq[(String, NDArray)]
   Default value removed: private val dataBatchSize: Int = 1 -> private val 
dataBatchSize: Int
   Default value removed: shuffle: Boolean = false -> shuffle: Boolean
   Default value removed: lastBatchHandle: String = "pad" -> lastBatchHandle: 
String
   Argument removed: dataName: String = "data"
   Argument removed: labelName: String = "label"
   ```
   
   Backwards compatibility means that users using version 1.0 of MXNet, should 
not receive any errors when they upgrade to version 1.X. In your PR, you're 
modifying the constructor. This would throw an error for all users if they 
upgrade to the latest version. 
   
   Instead, you should add an additional overload of the constructor in order 
to support the old constructor as well as providing a new one using your 
proposed method. Speak, don't modify existing constructors but rather create 
another one.


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

[GitHub] marcoabreu commented on issue #9771: Modify NDArrayIter constructor to receive tuple (i.e. dict in Python)?

[GitBox]

marcoabreu commented on issue #9771: Modify NDArrayIter constructor to receive 
tuple (i.e. dict in Python)?
URL: https://github.com/apache/incubator-mxnet/pull/9771#issuecomment-365201632
 
 
   Since we are using semantic versioning, this would require a major version
   update. Thus, this change can not be accepted in that form. I'd appreciate
   it if you could try to find a backwards compatible solution. If you need
   help in doing so, feel free to let us know and we will try to assist.
   
   Am 13.02.2018 3:28 vorm. schrieb "Sheng-Ying" :
   
   Adding an additional constructor is our first consideration. But after
   observing the original interface:
   
   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 {...
   
   we found there is no position to place the custom names. Therefore, since
   each overloading constructor has to call primary constructor, keeping the
   original interface may not a possible way if we want to make using multiple
   input flexible.
   
   Another reason we modifying the existing constructor is that we observer
   the fields defined in the class:
   
   val initData: IndexedSeq[(String, NDArray)] = IO.initData(_dataList,
   false, dataName)val initLabel: IndexedSeq[(String, NDArray)] =
   IO.initData(_labelList, true, labelName)
   
   We guess maybe using the type IndexedSeq[(String, NDArray)] is the original
   motivation at the beginning.
   
   In Python there is a way
   

   to manipulate multiple input. But Scala is static type. Maybe we can try to
   rethink about the API interface design.
   
   ?
   You are receiving this because you commented.
   Reply to this email directly, view it on GitHub
   ,
   or mute the thread
   

   .
   


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

[GitHub] marcoabreu commented on issue #9771: Modify NDArrayIter constructor to receive tuple (i.e. dict in Python)?

[GitBox]

marcoabreu commented on issue #9771: Modify NDArrayIter constructor to receive 
tuple (i.e. dict in Python)?
URL: https://github.com/apache/incubator-mxnet/pull/9771#issuecomment-365043493
 
 
   Please consider adding an additional constructor instead of modifying the 
existing constructor to provide backwards compatibility


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