Skip to content

Support more definitions of the Laplacian operator#7888

Draft
lagru wants to merge 3 commits intoscikit-image:mainfrom
lagru:flexible-laplace-operator
Draft

Support more definitions of the Laplacian operator#7888
lagru wants to merge 3 commits intoscikit-image:mainfrom
lagru:flexible-laplace-operator

Conversation

@lagru
Copy link
Copy Markdown
Member

@lagru lagru commented Sep 1, 2025

Description

Closes #7357, closes #7724, supersedes #7366.

Checklist

Release note

For maintainers and optionally contributors, please refer to the instructions on how to document this PR for the release notes.

...

@lagru lagru added ⏩ type: Enhancement Improve existing features 🥾 Path to skimage2 A step towards the new "API 2.0" labels Sep 1, 2025
Comment on lines +717 to 720
_, laplace_op = laplacian(
image.ndim, shape=(ksize,) * image.ndim, connectivity=connectivity, sign=sign
)
result = convolve(image, laplace_op)
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just noticed that the shape=... argument seems to have no effect on the current implementation on main.

impr = np.zeros([3] * ndim)
for dim in range(ndim):
idx = tuple(
[slice(1, 2)] * dim + [slice(None)] + [slice(1, 2)] * (ndim - dim - 1)
)
impr[idx] = np.array([-1.0, 0.0, -1.0]).reshape(
[-1 if i == dim else 1 for i in range(ndim)]
)
impr[(slice(1, 2),) * ndim] = 2.0 * ndim
return ir2tf(impr, shape, is_real=is_real), impr

only uses shape to compute the transfer function, which isn't used here... So the function making that available is confusing?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@glemaitre, do you remember more about this from 41875cf? I currently think this was probably a misunderstanding.

So I think we can safely deprecate this argument?

@lagru lagru moved this to Pitched in Work pitches Mar 12, 2026
@lagru lagru self-assigned this Mar 12, 2026
of `connectivity` from the center are considered neighbors.
`connectivity` may range from 1 (no diagonal neighbors) to `ndim` (all
neighbors are included).
sign : {-1, 1}, optional
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to provide some more information here on why one might want one or the other.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hope we're setting the default correctly; default is not indicated here in the docstring. IIRC we wanted to flip it from our current definition?

@lagru lagru moved this from Pitched to Approved in Work pitches Mar 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

⏩ type: Enhancement Improve existing features 🥾 Path to skimage2 A step towards the new "API 2.0"

Projects

Status: Approved

Development

Successfully merging this pull request may close these issues.

Add option to flip Laplacian sign filters.laplace result has the wrong sign

2 participants