Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Laplacian filter wrong sign #7357 #7366

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pitkajuh
Copy link
Contributor

@pitkajuh pitkajuh commented Apr 3, 2024

Description

This pull request fixes #7357

Checklist

Release note

We use changelist to
compile each pull request into an item of the release notes. Please refer to
the instructions
and past release notes
for guidance and examples.

Fix the inverted signs in `filters.laplace`.

@mkcor mkcor added the 🩹 type: Bug fix Fixes unexpected or incorrect behavior label Apr 4, 2024
@mkcor
Copy link
Member

mkcor commented Apr 4, 2024

Hi @pitkajuh,

Thanks for tackling this bug! Please update the tests accordingly and document the changes in comments, e.g., by referring to the SciPy implementation (as done in #7357).

@pitkajuh pitkajuh force-pushed the fix-7357-laplacian branch 2 times, most recently from 0241ae4 to a384418 Compare April 13, 2024 15:49
@pitkajuh
Copy link
Contributor Author

Hi @pitkajuh,

Thanks for tackling this bug! Please update the tests accordingly and document the changes in comments, e.g., by referring to the SciPy implementation (as done in #7357).

Hello. Yes, sorry about that. I fixed the tests and now they should work. The problem was just as described in #7357. Removing and adding minus signs from/to appropriate places fixed the issue and now the result are identical to the SciPy implementation.

@mkcor
Copy link
Member

mkcor commented May 15, 2024

@lagru since we are changing the return value of a function, I think we should deprecate this function and replace it with, say, ski.filters.laplacian so that users know they should update/check current code with ski.filters.laplace(arr). What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🩹 type: Bug fix Fixes unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

filters.laplace result has the wrong sign
2 participants