Bnb/min padding general #258
Conversation
…ate models with increased receptive field and larger padding values.
…adding`` layers in the generator model. Generalized current min padding to use these values.
sup3r/pipeline/slicer.py
Outdated
| if 2 * self.spatial_pad + (lr_slice_stop - lr_slice_start) < 4: | ||
| if ( | ||
| 2 * padding + (lr_slice_stop - lr_slice_start) | ||
| <= self.min_width[dim] |
There was a problem hiding this comment.
Personally, i find it really helpful to define some of these weird conditionals, both for readability and also to reduce this crazy one-liner. What are these? Just guessing but i'd do:
slice_width = lr_slice_stop - lr_slice_start
padded_width = 2 * padding + slice_width
too_small = padded_width <= self.min_width[dim]
if too_small:
...
And also wait i would think that if we're padded_width == min_width we should be okay based on the docstring for "min_width"? Surprised that it's <=
There was a problem hiding this comment.
Yeah good suggestions. It's <= because the method in strategy returns the .paddings attribute, and the min padding needs to be 1 more than this. But we can just do < and make it an arg.
sup3r/pipeline/slicer.py
Outdated
| check_boundary | ||
| and win_stop == max_steps | ||
| and (2 * max_pad + win_stop - win_start) < 4 | ||
| and (2 * max_pad + win_stop - win_start) <= min_width |
There was a problem hiding this comment.
same comment here, i'd recommend breaking this into semantic variables thats easier for future us to read.
sup3r/pipeline/strategy.py
Outdated
| pad_width = [ | ||
| np.max((new_pw[i], pad_width[i])) for i in range(3) | ||
| ] | ||
| return pad_width |
There was a problem hiding this comment.
This feels like it could be a bit specific to our current model design. What if we migrate to a different padding layer or change the attr in FlexiblePadding? Seems like a pretty rigid tight coupling. Could we just have min_width be an optional user arg that defaults to (1,1,1)?
There was a problem hiding this comment.
Yeah that was roughly the thinking with the implicit (1,1,1) default here but removing this method and making it a user kwarg is fine. I'd prefer the default to be (4,4,4) though, since we mostly use paddings = [3,3,3].
…layers. Cleaned up conditionals and doc strings.
Bnb/min padding general
2 participants
This generalizes the padding fix implemented in #247