Tightened type-checking of color input #393
Conversation
|
When tests pass I am going to update the changelog. |
for more information, see https://pre-commit.ci
…ta-plot into fix-isinstance
src/spatialdata_plot/pl/basic.py
Outdated
| @@ -950,8 +950,10 @@ def show( | |||
|
|
|||
| if wanted_labels_on_this_cs: | |||
| if (table := params_copy.table_name) is not None: | |||
| colors = sc.get.obs_df(sdata[table], params_copy.color) | |||
| if isinstance(colors[params_copy.color].dtype, pd.CategoricalDtype): | |||
| keys = [params_copy.color] if not isinstance(params_copy.color, list) else params_copy.color | |||
There was a problem hiding this comment.
I got lost on the type of params_copy.color. Is this always a str? Or is it sometimes an Iterable? Can it be of len() > 1 here? The tests pass but if we know it's always a string we can write better code.
There was a problem hiding this comment.
I'm pretty sure it can never be an Iterable, but would like to check more carefully before really relying on this
There was a problem hiding this comment.
it can never be an interable, it is always either color like or it corresponds to a particular column that then contains the color. This is the case on when it is provided by the user.
src/spatialdata_plot/pl/render.py
Outdated
| @@ -453,9 +453,11 @@ def _render_points( | |||
| ) | |||
|
|
|||
| if col_for_color is not None: | |||
| cols = sc.get.obs_df(adata, col_for_color) | |||
| keys = [col_for_color] if not isinstance(col_for_color, list) else col_for_color | |||
There was a problem hiding this comment.
Same comment as above for col_for_color, which is defined as render_params.col_for_color
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #393 +/- ##
==========================================
+ Coverage 83.52% 83.54% +0.01%
==========================================
Files 8 8
Lines 1687 1689 +2
==========================================
+ Hits 1409 1411 +2
Misses 278 278
|
|
@timtreis @Sonja-Stockhaus @melonora very small PR (most of the changes are from a pre-commit) that fixes a bug I got locally a while ago, can you please have a look? |
|
Test pass, so for the sake of a release I'd be inclined to merge soon and eventually come back to this. |
There was a problem hiding this comment.
Lgtm, if you want to merge soon @LucaMarconato, we can check the str/Iterable thing in a later PR
There was a problem hiding this comment.
Can you please provide first a small reproducible example that shows the issue is still there on main because I think I already implemented a fix for this a while back. Main reason was that the scanpy function being used had a different return type. Not too certain about it but I would like to see an example first.
|
Thanks for the review, I will try to find the plot that was causing the problem. |
|
Ok I looked more into this. What caused the problem is due to We could also fix this by requiring |
for more information, see https://pre-commit.ci
|
I tested it against |
|
Merging since the problem is now addressed. |
Small fix. I while ago I started getting an error here:
because
colors[params_copy.color].dtypewas a dataframe instead of a series, and thereforedtypewas not present. I lost the code I used to reproduce this because I had put together a solution quickly while being busy in another PR.The behavior is due to the fact that
colors = sc.get.obs_df(some_adata, some_keys)is apd.DataFrameifsome_keysis anIterable, while it is apd.Serieswhensome_keysis astr(this is howsc.get.obs_df()behaves).