search

gonum / matrix

Matrix packages for the Go language [DEPRECATED]
446 stars 53 forks source link

mat64: fix shadow boundary case #360

Closed kortschak closed 8 years ago

kortschak commented 8 years ago

Fixes #359.

btracey commented 8 years ago

I don't understand. Could you elaborate on what the problem was? This seems to be subtler than I originally realized, so I think the best place to elaborate is in a function comment. Thanks.

kortschak commented 8 years ago

Added commentary. PTAL

kortschak commented 8 years ago

To be explicit here about the problem, previously rectanglesOverlap did not properly handle the case where the later matrix (b) has a right column that is at the far right of the strided backing slice. This meant it was handled by the wrapped case and caused the overlap to be missed. The comments in the code now should allow you to draw what things will look like (the only way to explain it properly is graphically, which I obviously can't do here).

ping also @vladimir-ch

btracey commented 8 years ago

LGTM, but I'd appreciate a glance from @vladimir-ch given I've already been wrong once about this snippet.

kortschak commented 8 years ago

Agreed.

vladimir-ch commented 8 years ago

Just four ints and so difficult to reason about. LGTM

kortschak commented 8 years ago

Yes. The comments make it much clearer and @btracey 's observation has significantly simplified the test.