8000 Add normalization argument to LearnTransform.Translation with default value None by iimog · Pull Request #2063 · spacetx/starfish · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add normalization argument to LearnTransform.Translation with default value None #2063

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
May 9, 2025
Merged

Add normalization argument to LearnTransform.Translation with default value None #2063

8000
merged 5 commits into from
May 9, 2025

Conversation

iimog
Copy link
Contributor
@iimog iimog commented Apr 1, 2025

these are passed to the internal call of skimage.registration.phase_cross_correlation. This allows users to overwrite the normalization argument to restore the skimage<0.19 behavior of normalization=None, thereby fixing: #2062

I replaced the unused *args with **kwargs. If the *args are still needed, I can instead add **kwargs.

I decided to add this in run because that required the least amount of change. But it might be a better idea to add it to init instead (or in addition). Therefore it is specified once when the Translation object is initialized (and can potentially be overwritten in the calls to run).

We might consider adding additional documentation or selecting normalization=True as the default.

@iimog iimog mentioned this pull request Apr 1, 2025
Closed
@shachafl
Copy link
Collaborator
shachafl commented Apr 2, 2025

Hey @iimog,
Thanks for catching that and preparing the PR.
You can blame me for the scikit-image upgrade from 0.18.3.
I did notice the change in phase_cross_correlation() and had to update a couple of test files, but for my case and the tests in starfish the differences weren't significant (see #2009).
Seeing your test example and thinking that most users would probably benefit better noise handling than uniform illumination issues. I think we should explicitly add the key-argument with default normalization=None to phase_cross_correlation() with a reference to the relevant section in scikit-image docs.
Ideally, you should also add your example to a test file, maybe starfish/core/image/_registration/LearnTransform/test/test_translation.py?

@berl what do you think?

@iimog
8000 Copy link
Contributor Author
iimog commented Apr 2, 2025

Hi @shachafl,
My pleasure. Actually, I want to thank you (rather than blaming you) for upgrading scikit-image. 😊 😄 This has been a lot of work, and I'm excited to see starfish development continue. Also, installation in a current Python environment is now much easier.
I'm happy to revise my pull request according to your suggestion. If I understand correctly, I'll add the normalization argument to init (potentially with default value None) and leave the run method unchanged. I'm also happy to add a test case and propose some documentation.

@iimog iimog marked this pull request as draft April 2, 2025 14:00
@shachafl
Copy link
Collaborator
shachafl commented Apr 3, 2025

Sounds good to me.
I think a default value None makes sense, at least until another user complains :-)

For documentation, besides the docstring in the Translation class, as a minimum I would recommend:

  1. starfish/examples/tutorials/plot_image_registration.py
  2. starfish/examples/quick_start/plot_quick_start.py

A word of caution, Apple Silicon (M1+) might show some differences in floating-point arithmetic compared to Intel, so if you can run on both that's great. I need to resolve some failed tests before adding MacOS to GitHub Actions (#2057).

@iimog
Copy link
Contributor Author
iimog commented Apr 11, 2025

Awesome, I'll start working on this next week.

@iimog
Copy link
Contributor Author
iimog commented May 6, 2025

I changed the code, tests, and documentation as discussed (and force-pushed).

However, one test is still failing: starfish/test/full_pipelines/api/test_iss_api.py. This test depends on the old default and not only the expected_registered_values but also the gene_counts are changing when using normalization=None as the new default. I wonder whether it is better to run the ISS pipeline setting normalization="phase" explicitly, rather than changing all the expectations. In other cases, I tested for both settings for normalization. @shachafl what do you think?

I'll also run the tests on an Apple Silicon machine later.

@shachafl
Copy link
Collaborator
shachafl commented May 6, 2025

@iimog thank you for the update.
When I upgraded scikit-image to ver 0.20 I updated the registration test part of test_iss_api.py as the normalization default changed: (02c654f)
I recommend reverting back (updating the test result) to support your change and the new default.

@iimog iimog changed the title Add kwargs to LearnTransform.Translation.run Add normalization argument to LearnTransform.Translation with default value None May 7, 2025
@iimog
Copy link
Contributor Author
iimog commented May 7, 2025

I updated the expectations accordingly. Restoring the old expected_registered_values did not work, apparently the expectation still changed from before the upgrade to scikit-image (but they are much closer with normalization=None and all the other values match the old expectation).

I don't know why the lint check fails, but it was not during the linting, but during the installation. So I don't think this is my fault. 🙂

@iimog iimog marked this pull request as ready for review May 7, 2025 14:04
@shachafl shachafl self-requested a review May 7, 2025 14:32
@shachafl
Copy link
Collaborator
shachafl commented May 8, 2025

It all runs fine on my machine.
There is an issue with the cache in Github actions. I have opened an issue actions/setup-python#1101.

@iimog
Copy link
Contributor Author
iimog commented May 8, 2025

I successfully ran the tests on an M3 Mac (running Sequoia 15.3.1). Only the napari tests are failing (pytest -m 'napari'), both on my Linux machine and on the Mac.

This has nothing to do with this pr, but one required change is edge_color to border_color here:

starfish/starfish/core/_display.py

Line 282 in b8fdfa9

edge_color="red",

edge_color was deprecated in napari 0.5 and is removed in napari 0.6 (current stable version since May 1). However four of the tests still fail

starfish/core/test/test_display.py FF..FF..                              [100%]

[...]

=========================== short test summary info ============================
FAILED starfish/core/test/test_display.py::test_display[stack-spots-masks] - ValueError: cannot convert float NaN to integer
FAILED starfish/core/test/test_display.py::test_display[stack-spots-     ] - ValueError: cannot convert float NaN to integer
FAILED starfish/core/test/test_display.py::test_display[     -spots-masks] - ValueError: cannot convert float NaN to integer
FAILED starfish/core/test/test_display.py::test_display[     -spots-     ] - ValueError: cannot convert float NaN to integer

shachafl
shachafl approved these changes May 8, 2025
View reviewed changes
Copy link
Collaborator
@shachafl shachafl left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good to me.
Thanks again for doing all the hard work!

Last thing, please open a new issue with the failed napari testing. I wasn't vigilant with the napari testing so far.

@shachafl shachafl linked an issue May 9, 2025 that may be closed by this pull request
Registration fails with latest starfish (and scikit-image) #2062
Closed
@shachafl shachafl merged commit 04ca0e3 into spacetx:master May 9, 2025
35 of 37 checks passed
This was referenced May 9, 2025
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shachafl shachafl shachafl approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Registration fails with latest starfish (and scikit-image)
2 participants
@iimog @shachafl
0