Editorial: Fix glitch with otherwise clause in pool op creation

[inexorabletash]

A pair of steps in "create pooling operation" had an "If" condition with multiple clauses, and a subsequent "Otherwise" step that needs to apply if only the first clause is true. Rework the lines to correct the logic, and also align with subsequent argument validation.

Also, put "Otherwise, ..." steps on their own line consistently.

---

Preview | Diff

[inexorabletash]

@huningxin @fdwr - super minor fix, can you please take a look?

But while I'm here... the step "set options.windowDimensions to the height and width dimensions of the shape of input." could be made more rigorous by actually giving the dimensions. This would be most easily done by pulling "calculate pool2d output sizes" steps inline so that inputHeight and inputWidth are calculated earlier given options.layout. I didn't want to do that without asking for opinions first. Thoughts?

[fdwr]

👍

[fdwr]

But while I'm here... the step "set options.windowDimensions to the height and width dimensions of the shape of input." could be made more rigorous by actually giving the dimensions. This would be most easily done by pulling "calculate pool2d output sizes" steps inline so that inputHeight and inputWidth are calculated earlier given options.layout. I didn't want to do that without asking for opinions first. Thoughts?

I'm unsure why "calculate pool2d output sizes" is separate from the main algorithm, since it doesn't appear to be commonly shared anywhere else, but maybe it's that way because these others are also in independent sections...

• calculate reduction output sizes
• calculate conv2d output sizes
• calculate convtranspose2d output sizes
• calculate matmul output sizes
• calculate padding output sizes

...and if we're keeping the output calculation separate in the other operators, then consistency is nice. Maybe we can refer to it rather than move inline?

[inexorabletash]

I'm unsure why "calculate pool2d output sizes" is separate from the main algorithm, since it doesn't appear to be commonly shared anywhere else, but maybe it's that way because these others are also in independent sections...

(snip)

...and if we're keeping the output calculation separate in the other operators, then consistency is nice.

I did a quick poll of my fellow Googlers a while back, and they were in favor of inlining algorithms that are used in only one place. I think with the Calculate the output shape: (etc) subsections in the algorithms we maintain readability

Maybe we can refer to it rather than move inline?

Yeah, I think the intent is pretty obvious right now. We could also pull just the "Switch on layout..." step out and pass batches, channels, inputHeight, and inputWidth in, rather than inputShape. But at that point there's not much left and the description of the algorithm inputs is almost as long as the algorithm itself.

[huningxin]

But while I'm here... the step "set options.windowDimensions to the height and width dimensions of the shape of input." could be made more rigorous by actually giving the dimensions.

+1

Maybe we can refer to it rather than move inline?

Yeah, I think the intent is pretty obvious right now. We could also pull just the "Switch on layout..." step out and pass batches, channels, inputHeight, and inputWidth in, rather than inputShape. But at that point there's not much left and the description of the algorithm inputs is almost as long as the algorithm itself.

I prefer to inline the "calculate pool2d output sizes" steps, because this algorithm internally calls "calculating conv2d output sizes" steps. By inline it, "pool2d" steps can be consistent with "conv2d" steps.

[huningxin]

LGTM!

[huningxin]

@inexorabletash , is this PR ready to merge? "calculate pool2d output sizes" change could be done by a separate CL, WDYT?

[inexorabletash]

@inexorabletash , is this PR ready to merge? "calculate pool2d output sizes" change could be done by a separate CL, WDYT?

Agreed, I'd prefer to merge this and do a follow-on. Thanks for fixing my glitch, too!