NetSpec: don't require lists to specify single-element repeated fields

[jeffdonahue]

This makes NetSpec slightly friendlier by allowing single-element repeated fields to be specified without lists. I like this because it's similar to how the protobuf text format works: a single-element repeated field can be specified just like an optional field, and backwards compatibility is maintained when an optional field is promoted to a repeated field. (To test this I modified an unrelated existing test -- happy to create a new one instead if that's desired.)

[longjon]

I did consider this interface, but decided against it. Happy to hear what others think, though.

• The cost of this convenience is another special case and another implicit type conversion.
• Forcing repeated fields to be lists forces the repeated-ness to be explicitly displayed, which improves discoverability.

On the other hand, this behavior mirrors the protobuf behavior (as text, but not the API), and is similar to the way numpy treats shapes (though n.b. not all repeated fields are shapes)...

[jeffdonahue]

Yeah, I understand the concerns; I think the best argument is that this mirrors the prototxt behavior and gives the optional -> repeated backward compatibility. If this isn't the way to go, I'd at least like to instead raise a nice exception when not using a list to specify a repeated field -- it's usually pretty hard for me to figure out what went wrong when I mess this up from the current backtrace.

[philkr]

I'd side with @jeffdonahue here. It would be a lot more convenient to be able to specify repeated fields with a scalar. If that's not possible, it would be nice to at least make the error handling a bit better. For example throw an exception the moment someone tries to assign a single value to a repeated field and not once to_proto is called.

[longjon]

@philkr as I'd expect :)

To be clear, what we're discussing here is type coercion. For shapes, this form of weak typing is accepted in the numpy API, although in general it makes me a bit queasy; we are essentially saying, in the EDSL language of specification, any value of type alpha is automatically coerced into type list of alpha.

Net spec, in my view, need not merely wrap protobuf; it's an opportunity to write the language we most want (within the constraints of being a Python EDSL, which has its own set of issues, which are different from those of protobuf). So the argument of mirroring protobuf behavior doesn't resonate very loudly with me; if we forget protobuf entirely, what should the language of nets look like?

Now I totally agree that exception handling needs dramatic improvements (PRs welcome!); that is one area that I just didn't get to on the first pass. At the very least, common errors ought to produce sensible messages. I also agree that it would be better to fail earlier, although that is a bit tricky in the current implementation, because protobuf generation is delayed as long as possible. (This is intentional, so that there is a complete non-protobuf representation of the net (suitable, the intention is, for easy processing and transformation), which means the specification doesn't meet the protobuf types until generation time.) Perhaps the types should be parsed and drawn into the Python graph IR to meet the specification, or perhaps we should just accept the protobuf IR from the beginning and generate it at once. For my own workflow, this hasn't been a huge deal, since I am never specifying a net without immediately calling to_proto (even while iterating in development), so I'll get the exceptions one way or another...

[longjon]

Could we note this behavior in the docstring?

[longjon]

Okay, to look a little closer at this, let's enumerate the cases where repeated fields are actually used in layer parameters. I count:

1. top and bottom names
2. loss weights
3. param specs
4. propagate_down
5. include/exclude rules
6. mean value for subtraction
7. fillers
8. blob shapes
9. Eltwise coefficients
10. Slice points

In addition, we'll have soon

A. N-D conv parameters

Of these, 6 and 7 have "broadcast" behavior, where specifying one is the same as specifying the same value n times for some otherwise determined n; 2, 3, 4, and 9 have "matching" behavior, where it's only legal to specify either zero or some otherwise determined n; 1, 5, 8, 10, and A have "specification" behavior, where their number is free (though note one won't really be specifying 1 this way in net spec, and can usually dispense with 5 as well).

• To express the "broadcast" behavior in Python, it's common to accept x for any size n but [x] only if n == 1 (see, e.g., scipy.ndimage.maximum_filter).
• For the "matching" behavior, note that what's being matched is, in all cases, an argument list (rather than some shaped data), for which it's common to write x in the unary case (note the imperfect analogy to *args).
• For the "specification" behavior, it's common to accept x as shorthand for [x] (see, e.g., numpy.zeros)

The first behavior is not maintained by either implementation, while the second and third would be implemented by this PR.

Given that (1) the distinction in these behaviors is not specified by protobuf, (2) neither implementation gives possibly "ideal" behavior in all cases, (3) the unary shorthand is widely accepted in the numpy/scipy APIs, and (4) there are no cases where the listless type has a potential different interpretation than the singleton list (though see parenthetical below), I see no good reason to reject the convenience of this PR.

(I'm still unclear on how A is meant to work in the 1-dimensional case, though; shouldn't kernel_size=[3] be a 1-d conv? Does kernel_size=[3, 3], pad=1 broadcast the padding or error out? Does the same behavior hold for higher-dimensional kernels?)

The patch looks good except documentation as noted.

[jeffdonahue]

Thanks for the detailed discussion @longjon! I'll add a note on the behavior to the docstring.

(I'm still unclear on how A is meant to work in the 1-dimensional case, though; shouldn't kernel_size=[3] be a 1-d conv? Does kernel_size=[3, 3], pad=1 broadcast the padding or error out? Does the same behavior hold for higher-dimensional kernels?)

For 1D conv, kernel_size=[3] (or now kernel_size=3) would specify length 3 kernels (and in any other dimension it specifies 3x3x...x3 kernels). kernel_size=[3,3] would error out for anything other than 2D conv. pad=1 would broadcast the padding to all spatial dims. (Not totally sure I understand what you're asking, let me know if I didn't answer your question.)

[shelhamer]

Thanks for the research and argument @longjon! I agree with everything here and think this is ready to merge following the docstring update by @jeffdonahue.

Merge of #2049 can follow.