devitocodes / devito

DSL and compiler framework for automated finite-differences and stencil computation
http://www.devitoproject.org
MIT License
562 stars 228 forks source link

`SubDimension` 'thickness' inconsitently defined #1183

Open rhodrin opened 4 years ago

rhodrin commented 4 years ago

Take a look at:

    @classmethod
    def left(cls, name, parent, thickness, local=True):
        lst, rst = cls._symbolic_thickness(name)
        return cls(name, parent,
                   left=parent.symbolic_min,
                   right=parent.symbolic_min+lst-1,
                   thickness=((lst, thickness), (rst, 0)),
                   local=local)

    @classmethod
    def right(cls, name, parent, thickness, local=True):
        lst, rst = cls._symbolic_thickness(name)
        return cls(name, parent,
                   left=parent.symbolic_max-rst+1,
                   right=parent.symbolic_max,
                   thickness=((lst, 0), (rst, thickness)),
                   local=local)

    @classmethod
    def middle(cls, name, parent, thickness_left, thickness_right, local=False):
        lst, rst = cls._symbolic_thickness(name)
        return cls(name, parent,
                   left=parent.symbolic_min+lst,
                   right=parent.symbolic_max-rst,
                   thickness=((lst, thickness_left), (rst, thickness_right)),
                   local=local)

The 'thickness' properties mean something different for middle vs left/right - this should probably be made consistent or the actual properties changed?

Note that this inconsistency results in 'SubDomain'.shape being computer incorrectly. A 'fix' is present in https://github.com/devitocodes/devito/tree/fix_subdim_size2 but I don't really like this and the underlying issue should probably be addressed now. I did start producing a 'proper' fix but it probably results in user code changing so wanted to discuss it here first.

mloubout commented 4 years ago

Maybe we do not need to change the user code but only how the input is processed to create the subdomain:

This would prbably mess up lot of sthings so maybe not as trivial as I picture it.

FabioLuporini commented 4 years ago

I'm looping in @tjb900 because it's a relevant discussion for him

I'll take a proper look tmr

rhodrin commented 4 years ago

Note: Temporary fix in #1186.

FabioLuporini commented 4 years ago

@rhodrin reminder that in your next PR you may wanna expand that comment in _arg_defaults with a reference to this issue