DickJC123 commented on issue #14998: Revert the change on broadcast_to param shape URL: https://github.com/apache/incubator-mxnet/pull/14998#issuecomment-495777701 I'm close to giving this the thumbs up (see the end of my comments). I think a PR relating to Shape serialization (as we have discussed) should be separate from this one because of the possible extended discussion, given the 1.5 release deadline and @fhieber's eagerness for this fix. On this PR, let me try one more time with my argument. Consider numpy broadcast_to() behavior: ``` Python 3.5.2 (default, Nov 12 2018, 13:43:14) [GCC 5.4.0 20160609] on linux Type "help", "copyright", "credits" or "license" for more information. >>> import numpy as np >>> x = np.array([1,2,3]) >>> np.broadcast_to(x, (3,3)) array([[1, 2, 3], [1, 2, 3], [1, 2, 3]]) >>> np.broadcast_to(x, (0,3)) array([], shape=(0, 3), dtype=int64) >>> ``` If we say that MXNet's broadcast_to() uses 0 to mean 'match input', regardless of compatibility mode, then we could never duplicate numpy's behavior. However, if we think of 0 to mean 'unknown' in non-compatibility mode, and we say that 'unknown' really means 'unknown until inputs are bound, then match input size', we'd retain numpy functionality: In numpy compatibility mode, MXNet could broadcast to a shape with a zero size as in: mx.sym.broadcast_to(x, (0,3)) and boadcast_to() an input-matching shape as in mx.sym.broadcast_to(x, (-1,3)) [this being an extension to numpy capability]. And speaking of which, is there a reason you chose -1 for unknown size in numpy compatibility mode? Why not None as in (None, 3) for the example above? If you chose None, then even MXNet reshape functionality could be considered a compatible superset of Numpy's (in compatibility mode): Numpy reshape: 0 = 0 size -1 = calculate dim so total elements match MXNet not in numpy-compatible mode: 0 = output size matches input [in contrast to numpy definition] -1 = calculate dim so total elements match -2 -> -4 other MXNet-specific definitions MXNet numpy-compatible mode: None = unknown size (until binding), then output size matches input 0 = 0 size [like numpy] -1 = calculate dim so total elements match [like numpy] -2 -> -4 other MXNet-specific definitions Having typed this up, I see one problem- what value holds 'None' size in the backend, if -1 might have a meaning as it does in reshape? It could be defined to have a non-conflicting value as in `enum ShapeSizeConstants { kUnknownSize = MIN_INT }` and tested via: ``` inline bool dim_size_is_known(const dim_t dim_size) { return dim_size != kUnknownSize; } ``` Anyway thanks for reading this far. Unless this really resonates with you, I'll leave it by saying *I support this PR as it stands*- this corrects many of the Sockeye test failures I was seeing (the rest related to Shape serialization, not broadcast_to()).
---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services
