-
Notifications
You must be signed in to change notification settings - Fork 17
make outline behavior more similar to squidpy #472
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
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #472 +/- ##
==========================================
- Coverage 83.89% 83.80% -0.09%
==========================================
Files 8 8
Lines 1819 1976 +157
==========================================
+ Hits 1526 1656 +130
- Misses 293 320 +27
🚀 New features to boost your workflow:
|
Note: when |
Regarding the "outline lies partly behind shape and will shine through if fill_alpha != 1" issue:
|
Updates: we introduced a Color class that stores colors together with an alpha value. |
…rse/spatialdata-plot into feature/outline_refactoring
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if a user specifies just the outline alpha but nothing else?
outline_color = render_params.outline_params.inner_outline_color.get_hex() | ||
ds_inner_outlines = ds.tf.shade( | ||
agg_inner_outlines, | ||
cmap=outline_color, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're just passing a hex to something that expects a cmap, what are the implications of this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the original typehint is "list of colors or matplotlib.colors.Colormap". So it's called cmap, but doesn't necessarily expect a colormap object. Colors can be specified by name, hex code or RGB arrays. If you only give one color, it doesn't have to be a list. So it shouldn't have implications for us
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it automagically converts single colors to a list under-the-hood here? Or should we pass in either a one-element list or a list with the color duplicated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…rse/spatialdata-plot into feature/outline_refactoring
For outsourcing the plotting from squidpy to spatialdata-plot: refactor the outlines of the shapes to be more similar to squidpy. This includes:
outline_width
andoutline_color
take either a single argument as before, or a tuple of two values referring to the outer and inner outline. If one of the two arguments indicates that 2 outlines should be drawn but the other is not used (or only contains of 1 value which would be interpreted as referring to the outer outline), the missing value(s) are set to the defaults which are for the outer outline: 1.5 and "black" and for the inner outline: 0.5 and "white". If the linewidth for the inner outline is wider than the one of the outer outline, it will hide the outer outline (if alpha = 1.0).