search

tum-pbs / PhiFlow

A differentiable PDE solving framework for machine learning
MIT License
1.43k stars 193 forks source link

Potential Bug regarding non-zero extrapolation padding in math.convolve #104

Closed Dxyk closed 1 year ago

Dxyk commented 1 year ago

Hello,

I've been trying this package out and it has been a blast. Thank you for the amazing work!

I'm currently playing around with grid value convolutions and have potentially stumbled upon a bug when the padding extrapolation mode is non-ZERO. The error is AssertionError: Shape.name is only defined for shapes of rank 1. shape=(xˢ=5, yˢ=5)

Here's a piece of code to reproduce the error.

values = math.random_normal(spatial(x=64, y=64))
kernel = math.random_normal(spatial(x=5, y=5))
values_conv = math.convolve(values, kernel, extrapolation.PERIODIC)

I think a potential fix is for this line, instead of attempting to loop over a single name for dim in conv_shape.name, loop over the names of the shape for dim in conv_shape.names. I tested locally with the example above and the assertion error goes away. I'm not entirely sure if this is the intended behaviour though.

https://github.com/tum-pbs/PhiFlow/blob/9d11b27dba48525e9b6a28161fa9768b4216d726/phi/math/_ops.py#L1759-L1761

Thanks for taking a look!

holl- commented 1 year ago

Yes, good catch. I've pushed this to 2.3-develop

You can install 2.3 with pip install --upgrade git+https://github.com/tum-pbs/PhiFlow@2.3-develop