Explicit padding mode about objax HOT 6 CLOSED

I like your proposal. If I understand correctly, in this new way of doing things a call to tf.pad would be added inside the code for Conv2D to handle more padding modes (like symmetric, reflect, constant padding)? And when not needed (zero-padding or valid) there would be no call to tf.pad (to keep performance optimized)? And the API change would also permits passing explicit padding intervals (integers) per axis.

So the convolution padding would be defined as (to define the "geometry" of the padding):

class ConvPadding(enum.Enum):
  """An Enum holding the possible padding values for convolution modules."""
    SAME = 'SAME'
    VALID = 'VALID'

And the mode (what values are used to fill the padding):

class ConvPaddingMode(enum.Enum):
  """An Enum holding the possible padding values for convolution modules."""
    SYM = 'SYM'  # specify symmetric padding for spatial dim as tuple for H, W or single int
    REFLECT = 'REFLECT'
    MEAN = 'MEAN'
    # ... and so on.

If I understand correctly, that's what you encapsulated in the proposed Padding dataclass.
I would prefer to pass two arguments to Conv2D.__init__ instead (to keep the code accessible, we voluntarily limit abstractions to keep the code more welcoming to people of varied CS experience).

ConvPaddingInt = Union[Sequence[Tuple[int, int]], Tuple[int, int], int]

class Conv2D(...):
    def __init__(self,
                 ...
                 padding: Union[ConvPadding, ConvPaddingInt] = ConvPadding.SAME,
                 padding_mode: Union[ConvPaddingMode, float] = 0,
                 ...

Would this variant of your initial proposal work for you?

from objax.

@david-berthelot TLDR yes, I think the above proposal would work great despite confusion over my poor choice of the term 'symmetric'

Regarding 'symmetric', I wasn't actually thinking modes while trying to jot down the idea. My additional enum values were all ways of specifying the 'geometries' (or 'padding type'). I was proposing different alternatives dataclass w/ extended enum vs union as I wasn't sure if single types were preferred over unions for interfaces here.

So 'symmetric' referred to the fact that the int and Tuple[int, int] result in symmetric amounts of padding -- pad_beg == pad_end with no ability to end up with beg != end as with 'SAME' padding.

As for the modes, that'd also be useful for some less common use cases and covered by calling pad before the conv as you mention. I use 'circular' and 'constant' from time to time, but not 'reflect', 'symmetric', 'replicate'.

from objax.

Would you be open to contributing a PR? If not please let me know; I, or someone else, can follow up.

from objax.

@david-berthelot I would be, currently I have part of the change (padding type, not the mode) intertwined with some others for my prototyping. Once I get a bit further on that and get a chance to test properly I can do a PR here. But if someone else wants to take it on here before that happens, that's a-ok.

from objax.

Here's the plan, I collected more opinions and we'll extend in the simplest way, I'll do the coding in the coming days.

This part will remain the same.

class ConvPadding(enum.Enum):
  """An Enum holding the possible padding values for convolution modules."""
    SAME = 'SAME'
    VALID = 'VALID'

Conv2D (and others) will be extended to support numeric padding.

ConvPaddingInt = Union[Sequence[Tuple[int, int]], Tuple[int, int], int]

class Conv2D(...):
    def __init__(self,
                 ...
                 padding: Union[ConvPadding, ConvPaddingInt] = ConvPadding.SAME,
                 ...

In the end, we decided againt added numpy-style mode paddings since we want to be mindful about keeping the original design simplicity.

from objax.

@david-berthelot makes a lot of sense, the numeric padding was the motivation for this issue. Fancy modes are much less common and can always be done with separate padding layers.

from objax.