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 the bug of relative coordinates #572

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

JiaShun-Xiao
Copy link

Description

I have changed the relative coordinate to exact coordinate in the feature_segmentation function.
There are two reasons:

  1. because of the normalization of "np.max(y) - np.min(y)" in denominator, current relative coordinate will return nan if there is only one cell in spot.
  2. exact coordinate looks more reasonable than the relative coordinate when visualizing the segmentation result with spatial image as background.

How has this been tested?

I have tested the revised code in two Visium data, it works very well.

@codecov-commenter
Copy link

codecov-commenter commented Jun 17, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.32%. Comparing base (2cf664f) to head (06964fc).
Report is 134 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #572      +/-   ##
==========================================
- Coverage   72.33%   72.32%   -0.02%     
==========================================
  Files          37       37              
  Lines        5177     5175       -2     
  Branches      982      982              
==========================================
- Hits         3745     3743       -2     
  Misses       1181     1181              
  Partials      251      251              
Files with missing lines Coverage Δ
squidpy/im/_feature_mixin.py 87.17% <100.00%> (-0.17%) ⬇️

@giovp
Copy link
Member

giovp commented Jun 18, 2022

hi @JiaShun-Xiao thanks for the cotnribution, can you elaborate a bit more and provide code snippets about this PR? I don't think I fully understand teh settigns in which this fails. Also, can you check out the contributing guide for pre-commit checks (currently failing).

Thank you!

@giovp
Copy link
Member

giovp commented Aug 9, 2022

hi @JiaShun-Xiao ,

could you please elaborate on the purpose of this PR and provide a minimal reproducible example?

Thank you!
GIovanni

@JiaShun-Xiao
Copy link
Author

Hi, @giovp

Thanks for your patience!
Current relative coordinates are fine in most cases. However, I think it could be improved by using exact coordinates as I mentioned in the first comment.
The nuclei segmentation example (https://squidpy.readthedocs.io/en/latest/external_tutorials/tutorial_tangram.html) provided in squidpy document can demonstrate this.

Thank you!
Jiashun

@giovp
Copy link
Member

giovp commented Aug 10, 2022

interesting ok, the tests seem to pass, could you add a small test with this specific edge case? Otherwise a minimal reproducible example here and I'd be happy to add it myself, thank you!

@JiaShun-Xiao
Copy link
Author

Sorry I am not familiar with tests in programming, I need some time to learn how to add tests for edge cases

@giovp
Copy link
Member

giovp commented Aug 10, 2022

hi @JiaShun-Xiao , no problem. Could you please then post here few lines of code that explain what is the problem you are trying to solve? ideally something that can be run directly, with minimal dependency (e.g. squidpy, numpy etc)

@JiaShun-Xiao
Copy link
Author

Sure, the problem I am trying to solve is retriving exact cell coordinates/locations after nuclei segmentation with spatial histological images.
To demonstrate this, I add an example in the following Jupiter notebook to illustrate why we need exact nuclei coordinates instead of relative coordinates.

squidpy-mouce-brain-MOp-demo.ipynb.zip

squidpy-mouce-brain-MOp-demo.pdf

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.

3 participants