Skip to content

Conversation

@selmanozleyen
Copy link
Member

Fixes #1014

@selmanozleyen
Copy link
Member Author

thanks for the feedback but fau doesn't work with python 3.10.

@selmanozleyen
Copy link
Member Author

@Intron7 should I merge this branch?

Copy link
Member

@flying-sheep flying-sheep left a comment

Choose a reason for hiding this comment

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

looks good to me, but @Intron7 should check if it works in his example.

@codecov
Copy link

codecov bot commented Nov 3, 2025

Codecov Report

❌ Patch coverage is 64.70588% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 66.60%. Comparing base (f11e20f) to head (a131158).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/squidpy/gr/_build.py 64.70% 5 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1028      +/-   ##
==========================================
+ Coverage   66.47%   66.60%   +0.13%     
==========================================
  Files          45       45              
  Lines        7015     7017       +2     
  Branches     1184     1185       +1     
==========================================
+ Hits         4663     4674      +11     
+ Misses       1890     1880      -10     
- Partials      462      463       +1     
Files with missing lines Coverage Δ
src/squidpy/gr/_build.py 88.47% <64.70%> (+3.83%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Intron7
Intron7 previously requested changes Nov 3, 2025
Length equals len(data). Entry-wise factors d_i * d_j * data[k]
"""

res = np.empty_like(data, dtype=np.float64)
Copy link
Member

Choose a reason for hiding this comment

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

why np.float64?

Copy link
Member Author

Choose a reason for hiding this comment

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

because original function used np.float64. Didn't want to change old behaviour

Copy link
Member

Choose a reason for hiding this comment

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

can we check if this is needed

Copy link
Member Author

Choose a reason for hiding this comment

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

well I can't tell tbh because there was no tests for the spectral transform lol. We can just use float32 I guess. But the better way would be to ask ths users of spatial_neighbours

Copy link
Member Author

Choose a reason for hiding this comment

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

@Intron7 ok the results seem similar when I checked them. I also added some numerical tests to ensure stability. Also when I recalled that this was just a normalization of weights in a graph so it should not be very sensitive to precision in theory.

@selmanozleyen
Copy link
Member Author

@timtreis could you merge this please? @Intron7 said he can't review it and @flying-sheep already had a look on this

@flying-sheep flying-sheep removed the request for review from Intron7 December 16, 2025 09:43
@selmanozleyen selmanozleyen merged commit c653810 into main Dec 16, 2025
12 checks passed
@selmanozleyen selmanozleyen deleted the fix/spectral-transform branch December 16, 2025 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

spatial_neighbors breaks when transform='spectral'

4 participants