Skip to content

add squidpy way to handle segment #45

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

Merged
merged 20 commits into from
Apr 12, 2023
Merged

add squidpy way to handle segment #45

merged 20 commits into from
Apr 12, 2023

Conversation

giovp
Copy link
Member

@giovp giovp commented Apr 5, 2023

No description provided.

@giovp
Copy link
Member Author

giovp commented Apr 9, 2023

@sagar87 @timtreis this is a heavy rewrite of the rendering functions. It mostly focuses on aesthetics (colors, legend, colorbars, etc.) and doesn't change the core behavior of the library.

With this current implementation I can showcase the aggregation functions in the notebooks but I'm afraid is not enough for the datasets you have been working on. I would however suggest to re-implement the functionalities in current logic.

There are also 2 critical changes that need to be made (not in this PR).

  • avoid copying of spatialdata. This is really important, the object must not be copied, but only views should be passed in the function call.
  • remove all transformations in a pp functionality. The way it's handled currently is quite confusing and happens in the show method.

Base automatically changed from feature/202303_translation_in_racoon to main April 10, 2023 09:59
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@giovp
Copy link
Member Author

giovp commented Apr 11, 2023

it's ready! it should have almost all functionalities as before + handle of caterogical and continuous for points.

Docstrings are missing, will try to finish them tomorrow.

@giovp giovp requested review from timtreis and sagar87 April 11, 2023 21:16
@codecov-commenter
Copy link

Codecov Report

Merging #45 (5831956) into main (b73f4aa) will increase coverage by 9.71%.
The diff coverage is 28.52%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #45      +/-   ##
==========================================
+ Coverage   30.62%   40.34%   +9.71%     
==========================================
  Files          12        9       -3     
  Lines         862      870       +8     
==========================================
+ Hits          264      351      +87     
+ Misses        598      519      -79     
Impacted Files Coverage Δ
src/spatialdata_plot/_accessor.py 100.00% <ø> (ø)
src/spatialdata_plot/pl/basic.py 20.32% <9.55%> (+9.85%) ⬆️
src/spatialdata_plot/pl/utils.py 28.43% <29.37%> (-4.90%) ⬇️
src/spatialdata_plot/pl/render.py 44.30% <41.49%> (+29.15%) ⬆️
src/spatialdata_plot/__init__.py 100.00% <100.00%> (ø)
src/spatialdata_plot/pp/basic.py 76.76% <100.00%> (-0.17%) ⬇️
src/spatialdata_plot/pp/utils.py 61.53% <100.00%> (ø)

@giovp giovp merged commit 4a50079 into main Apr 12, 2023
@giovp giovp deleted the new-segment branch April 12, 2023 11:26
@timtreis timtreis linked an issue Apr 12, 2023 that may be closed by this pull request
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.

Consider using NamedTuple instead of dictionaries
4 participants