devito icon indicating copy to clipboard operation
devito copied to clipboard

`SubDimension` 'thickness' inconsitently defined

Open rhodrin opened this issue 4 years ago • 4 comments

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.

rhodrin avatar Mar 23 '20 22:03 rhodrin

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

  • Turn the thickness left/right into a dim_width
  • Remove thickness internally
  • Size is just dim_width

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

mloubout avatar Mar 24 '20 11:03 mloubout

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

I'll take a proper look tmr

FabioLuporini avatar Mar 24 '20 16:03 FabioLuporini

Note: Temporary fix in #1186.

rhodrin avatar Mar 24 '20 18:03 rhodrin

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

FabioLuporini avatar May 19 '20 12:05 FabioLuporini