Fix var plotting by melonora · Pull Request #236 · scverse/spatialdata-plot

melonora · 2024-04-01T09:32:27Z

closes#234
Follow up on #235 fixing the plotting color columns that are numerical. First this fixes the visibility of the colorbar when plotting labels with a color. The issue here was that due to the outline_alpha being set to 0 when outline is False and outline of the label being plotted after the labels it would overwrite the colorbar. This was fixed by first plotting outline and then the label. A potential issue here is that when both alpha and outline alpha are 1 the outline will not be visible, but currently I thought this best as there is no clear way to decouple the colorbar from the outline. The general rule here is whatever is plotted last is visible.

The other issue got reported by @LucaMarconato in #234 . Ultimately, this was an issue when plotting feature0 in the models2 notebook in spatialdata-notebooks. The issue here was that 3 shapes are being plotted, one multipolygon and 2 polygons. This results in a color_vector of length 3 which is convertible to a RGB color, causing 1 color to be created for 3 shapes. This PR fixes the issue by checking whether we are dealing with a 1 dimensional color vector of length 3 or 4 and whether it then has the same length as the number of shapes.

The plot in models2 would now look like this:

codecov-commenter · 2024-04-01T10:02:33Z

Codecov Report

❌ Patch coverage is 50.00000% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.70%. Comparing base (9ddebfe) to head (46448b9).
⚠️ Report is 344 commits behind head on main.

Files with missing lines	Patch %	Lines
src/spatialdata_plot/pl/utils.py	38.46%	8 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #236 +/- ##
==========================================
- Coverage 78.01% 77.70% -0.31% 
==========================================
Files 11 11 Lines 1710 1718 +8 ==========================================
+ Hits 1334 1335 +1 - Misses 376 383 +7

Files with missing lines	Coverage Δ
src/spatialdata_plot/pl/render.py	`94.61% <100.00%> (ø)`
src/spatialdata_plot/pl/utils.py	`66.66% <38.46%> (-0.47%)`	⬇️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

melonora · 2024-04-01T12:58:07Z

lovely, tests in #237 do pass so we can ignore the failing test here.

giovp

ok for me ~~but maybe a very small test that would have caught this?~~ ah nvm ok with #237

LucaMarconato · 2024-04-04T13:24:37Z

Thanks @melonora for the PR! I noticed that neither main neither this PR is able to run the technology_mibitof.ipynb notebook because of a plot of a categorical into labels that fails.

I see a similar test is failing in this PR, so probably worth fixing them both, could you have a look at it please? After this we can merge.

melonora · 2024-04-04T13:26:23Z

Yeah will fix it when at the airport

melonora · 2024-04-04T14:05:35Z

hmm wait this PR was fine as the follow up PR the tests just passed. However with the last change on the follow up PR it is failing

melonora · 2024-04-04T14:23:12Z

Yeah as you can see just simply rerunning the failed job here makes it pass so here the issue was not a bug in the code.

timtreis

very few minor things

timtreis · 2024-04-04T14:35:51Z

 cax: PatchCollection,
 fig_params: FigParams,
- value_to_plot: str | None,
+ value_to_plot: str | None, # str | None,


timtreis · 2024-04-04T14:37:44Z

- element_table_mapping[element].add(table_name)
+ if isinstance(mapping := element_table_mapping[element], set):
+ mapping.add(table_name)
+ assert isinstance(element_table_mapping, dict)


I personally dislike spurious asserts. Can you verify before and block this path preemptively? If not, rather throw an informative warning

This is purely due to mypy not accepting anything else. The only way to get around this is by adopting the whole code base to use params per element.

Mypy gets very confused because of the data structure constantly changing. I tried casting but in most cases this does not help

timtreis · 2024-04-04T14:38:06Z

+ params.element_table_mapping
+ )
+
+ assert isinstance(element_table_mapping, dict)


timtreis · 2024-04-04T14:38:38Z

+ params.element_table_mapping
+ )
+
+ assert isinstance(element_table_mapping, dict)


timtreis · 2024-04-04T14:42:44Z

 else:
- if len(element_table_mapping[element_name].copy()) != 0:
- for table_name in element_table_mapping[element_name].copy():
+ if isinstance(mapping := element_table_mapping[element_name], set) and len(mapping.copy()) != 0:


reads a bit code-golf-y 👀 Also, why are you copying? Theoretically, you could also remove the != 0 since the len of an empty set is also 0 (False)

again partially mypy as for the copy, if you remove from a set during a for loop you will get an error because indices are removed while looping over the object. Here the one in len is unnecessary though so removing that.

timtreis · 2024-04-04T14:43:23Z

- if len(element_table_mapping[element_name].copy()) != 0:
- for table_name in element_table_mapping[element_name].copy():
+ if isinstance(mapping := element_table_mapping[element_name], set) and len(mapping.copy()) != 0:
+ for table_name in mapping.copy():


These are shallow copies no? Why do you do that?

timtreis · 2024-04-04T14:43:54Z

 params.col_for_color = [None] * len(render_elements)
 else:
+ assert isinstance(params.color, list)
+ assert isinstance(params.col_for_color, list)


melonora · 2024-04-04T15:50:37Z

Merging after talking with @timtreis

melonora · 2024-04-01T09:32:27Z

closes#234
Follow up on #235 fixing the plotting color columns that are numerical. First this fixes the visibility of the colorbar when plotting labels with a color. The issue here was that due to the outline_alpha being set to 0 when outline is False and outline of the label being plotted after the labels it would overwrite the colorbar. This was fixed by first plotting outline and then the label. A potential issue here is that when both alpha and outline alpha are 1 the outline will not be visible, but currently I thought this best as there is no clear way to decouple the colorbar from the outline. The general rule here is whatever is plotted last is visible.

The other issue got reported by @LucaMarconato in #234 . Ultimately, this was an issue when plotting feature0 in the models2 notebook in spatialdata-notebooks. The issue here was that 3 shapes are being plotted, one multipolygon and 2 polygons. This results in a color_vector of length 3 which is convertible to a RGB color, causing 1 color to be created for 3 shapes. This PR fixes the issue by checking whether we are dealing with a 1 dimensional color vector of length 3 or 4 and whether it then has the same length as the number of shapes.

The plot in models2 would now look like this:

codecov-commenter · 2024-04-01T10:02:33Z

Codecov Report

❌ Patch coverage is 50.00000% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.70%. Comparing base (9ddebfe) to head (46448b9).
⚠️ Report is 344 commits behind head on main.

Files with missing lines	Patch %	Lines
src/spatialdata_plot/pl/utils.py	38.46%	8 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #236 +/- ##
==========================================
- Coverage 78.01% 77.70% -0.31% 
==========================================
Files 11 11 Lines 1710 1718 +8 ==========================================
+ Hits 1334 1335 +1 - Misses 376 383 +7

Files with missing lines	Coverage Δ
src/spatialdata_plot/pl/render.py	`94.61% <100.00%> (ø)`
src/spatialdata_plot/pl/utils.py	`66.66% <38.46%> (-0.47%)`	⬇️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

melonora · 2024-04-01T12:58:07Z

lovely, tests in #237 do pass so we can ignore the failing test here.

giovp

ok for me ~~but maybe a very small test that would have caught this?~~ ah nvm ok with #237

LucaMarconato · 2024-04-04T13:24:37Z

Thanks @melonora for the PR! I noticed that neither main neither this PR is able to run the technology_mibitof.ipynb notebook because of a plot of a categorical into labels that fails.

I see a similar test is failing in this PR, so probably worth fixing them both, could you have a look at it please? After this we can merge.

melonora · 2024-04-04T13:26:23Z

Yeah will fix it when at the airport

melonora · 2024-04-04T14:05:35Z

hmm wait this PR was fine as the follow up PR the tests just passed. However with the last change on the follow up PR it is failing

melonora · 2024-04-04T14:23:12Z

Yeah as you can see just simply rerunning the failed job here makes it pass so here the issue was not a bug in the code.

timtreis

very few minor things

timtreis · 2024-04-04T14:35:51Z

 cax: PatchCollection,
 fig_params: FigParams,
- value_to_plot: str | None,
+ value_to_plot: str | None, # str | None,


timtreis · 2024-04-04T14:37:44Z

- element_table_mapping[element].add(table_name)
+ if isinstance(mapping := element_table_mapping[element], set):
+ mapping.add(table_name)
+ assert isinstance(element_table_mapping, dict)


I personally dislike spurious asserts. Can you verify before and block this path preemptively? If not, rather throw an informative warning

This is purely due to mypy not accepting anything else. The only way to get around this is by adopting the whole code base to use params per element.

Mypy gets very confused because of the data structure constantly changing. I tried casting but in most cases this does not help

timtreis · 2024-04-04T14:38:06Z

+ params.element_table_mapping
+ )
+
+ assert isinstance(element_table_mapping, dict)


timtreis · 2024-04-04T14:38:38Z

+ params.element_table_mapping
+ )
+
+ assert isinstance(element_table_mapping, dict)


timtreis · 2024-04-04T14:42:44Z

 else:
- if len(element_table_mapping[element_name].copy()) != 0:
- for table_name in element_table_mapping[element_name].copy():
+ if isinstance(mapping := element_table_mapping[element_name], set) and len(mapping.copy()) != 0:


reads a bit code-golf-y 👀 Also, why are you copying? Theoretically, you could also remove the != 0 since the len of an empty set is also 0 (False)

again partially mypy as for the copy, if you remove from a set during a for loop you will get an error because indices are removed while looping over the object. Here the one in len is unnecessary though so removing that.

timtreis · 2024-04-04T14:43:23Z

- if len(element_table_mapping[element_name].copy()) != 0:
- for table_name in element_table_mapping[element_name].copy():
+ if isinstance(mapping := element_table_mapping[element_name], set) and len(mapping.copy()) != 0:
+ for table_name in mapping.copy():


These are shallow copies no? Why do you do that?

timtreis · 2024-04-04T14:43:54Z

 params.col_for_color = [None] * len(render_elements)
 else:
+ assert isinstance(params.color, list)
+ assert isinstance(params.col_for_color, list)


melonora · 2024-04-04T15:50:37Z

Merging after talking with @timtreis

melonora · 2024-04-01T09:32:27Z

closes#234
Follow up on #235 fixing the plotting color columns that are numerical. First this fixes the visibility of the colorbar when plotting labels with a color. The issue here was that due to the outline_alpha being set to 0 when outline is False and outline of the label being plotted after the labels it would overwrite the colorbar. This was fixed by first plotting outline and then the label. A potential issue here is that when both alpha and outline alpha are 1 the outline will not be visible, but currently I thought this best as there is no clear way to decouple the colorbar from the outline. The general rule here is whatever is plotted last is visible.

The other issue got reported by @LucaMarconato in #234 . Ultimately, this was an issue when plotting feature0 in the models2 notebook in spatialdata-notebooks. The issue here was that 3 shapes are being plotted, one multipolygon and 2 polygons. This results in a color_vector of length 3 which is convertible to a RGB color, causing 1 color to be created for 3 shapes. This PR fixes the issue by checking whether we are dealing with a 1 dimensional color vector of length 3 or 4 and whether it then has the same length as the number of shapes.

The plot in models2 would now look like this:

codecov-commenter · 2024-04-01T10:02:33Z

Codecov Report

❌ Patch coverage is 50.00000% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.70%. Comparing base (9ddebfe) to head (46448b9).
⚠️ Report is 344 commits behind head on main.

Files with missing lines	Patch %	Lines
src/spatialdata_plot/pl/utils.py	38.46%	8 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #236 +/- ##
==========================================
- Coverage 78.01% 77.70% -0.31% 
==========================================
Files 11 11 Lines 1710 1718 +8 ==========================================
+ Hits 1334 1335 +1 - Misses 376 383 +7

Files with missing lines	Coverage Δ
src/spatialdata_plot/pl/render.py	`94.61% <100.00%> (ø)`
src/spatialdata_plot/pl/utils.py	`66.66% <38.46%> (-0.47%)`	⬇️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

melonora · 2024-04-01T12:58:07Z

lovely, tests in #237 do pass so we can ignore the failing test here.

giovp

ok for me ~~but maybe a very small test that would have caught this?~~ ah nvm ok with #237

LucaMarconato · 2024-04-04T13:24:37Z

Thanks @melonora for the PR! I noticed that neither main neither this PR is able to run the technology_mibitof.ipynb notebook because of a plot of a categorical into labels that fails.

I see a similar test is failing in this PR, so probably worth fixing them both, could you have a look at it please? After this we can merge.

melonora · 2024-04-04T13:26:23Z

Yeah will fix it when at the airport

melonora · 2024-04-04T14:05:35Z

hmm wait this PR was fine as the follow up PR the tests just passed. However with the last change on the follow up PR it is failing

melonora · 2024-04-04T14:23:12Z

Yeah as you can see just simply rerunning the failed job here makes it pass so here the issue was not a bug in the code.

timtreis

very few minor things

timtreis · 2024-04-04T14:35:51Z

 cax: PatchCollection,
 fig_params: FigParams,
- value_to_plot: str | None,
+ value_to_plot: str | None, # str | None,


timtreis · 2024-04-04T14:37:44Z

- element_table_mapping[element].add(table_name)
+ if isinstance(mapping := element_table_mapping[element], set):
+ mapping.add(table_name)
+ assert isinstance(element_table_mapping, dict)


I personally dislike spurious asserts. Can you verify before and block this path preemptively? If not, rather throw an informative warning

This is purely due to mypy not accepting anything else. The only way to get around this is by adopting the whole code base to use params per element.

Mypy gets very confused because of the data structure constantly changing. I tried casting but in most cases this does not help

timtreis · 2024-04-04T14:38:06Z

+ params.element_table_mapping
+ )
+
+ assert isinstance(element_table_mapping, dict)


timtreis · 2024-04-04T14:38:38Z

+ params.element_table_mapping
+ )
+
+ assert isinstance(element_table_mapping, dict)


timtreis · 2024-04-04T14:42:44Z

 else:
- if len(element_table_mapping[element_name].copy()) != 0:
- for table_name in element_table_mapping[element_name].copy():
+ if isinstance(mapping := element_table_mapping[element_name], set) and len(mapping.copy()) != 0:


reads a bit code-golf-y 👀 Also, why are you copying? Theoretically, you could also remove the != 0 since the len of an empty set is also 0 (False)

again partially mypy as for the copy, if you remove from a set during a for loop you will get an error because indices are removed while looping over the object. Here the one in len is unnecessary though so removing that.

timtreis · 2024-04-04T14:43:23Z

- if len(element_table_mapping[element_name].copy()) != 0:
- for table_name in element_table_mapping[element_name].copy():
+ if isinstance(mapping := element_table_mapping[element_name], set) and len(mapping.copy()) != 0:
+ for table_name in mapping.copy():


These are shallow copies no? Why do you do that?

timtreis · 2024-04-04T14:43:54Z

 params.col_for_color = [None] * len(render_elements)
 else:
+ assert isinstance(params.color, list)
+ assert isinstance(params.col_for_color, list)


melonora · 2024-04-04T15:50:37Z

Merging after talking with @timtreis

melonora · 2024-04-01T09:32:27Z

closes#234
Follow up on #235 fixing the plotting color columns that are numerical. First this fixes the visibility of the colorbar when plotting labels with a color. The issue here was that due to the outline_alpha being set to 0 when outline is False and outline of the label being plotted after the labels it would overwrite the colorbar. This was fixed by first plotting outline and then the label. A potential issue here is that when both alpha and outline alpha are 1 the outline will not be visible, but currently I thought this best as there is no clear way to decouple the colorbar from the outline. The general rule here is whatever is plotted last is visible.

The other issue got reported by @LucaMarconato in #234 . Ultimately, this was an issue when plotting feature0 in the models2 notebook in spatialdata-notebooks. The issue here was that 3 shapes are being plotted, one multipolygon and 2 polygons. This results in a color_vector of length 3 which is convertible to a RGB color, causing 1 color to be created for 3 shapes. This PR fixes the issue by checking whether we are dealing with a 1 dimensional color vector of length 3 or 4 and whether it then has the same length as the number of shapes.

The plot in models2 would now look like this:

codecov-commenter · 2024-04-01T10:02:33Z

Codecov Report

❌ Patch coverage is 50.00000% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.70%. Comparing base (9ddebfe) to head (46448b9).
⚠️ Report is 344 commits behind head on main.

Files with missing lines	Patch %	Lines
src/spatialdata_plot/pl/utils.py	38.46%	8 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #236 +/- ##
==========================================
- Coverage 78.01% 77.70% -0.31% 
==========================================
Files 11 11 Lines 1710 1718 +8 ==========================================
+ Hits 1334 1335 +1 - Misses 376 383 +7

Files with missing lines	Coverage Δ
src/spatialdata_plot/pl/render.py	`94.61% <100.00%> (ø)`
src/spatialdata_plot/pl/utils.py	`66.66% <38.46%> (-0.47%)`	⬇️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

melonora · 2024-04-01T12:58:07Z

lovely, tests in #237 do pass so we can ignore the failing test here.

giovp

ok for me ~~but maybe a very small test that would have caught this?~~ ah nvm ok with #237

LucaMarconato · 2024-04-04T13:24:37Z

Thanks @melonora for the PR! I noticed that neither main neither this PR is able to run the technology_mibitof.ipynb notebook because of a plot of a categorical into labels that fails.

I see a similar test is failing in this PR, so probably worth fixing them both, could you have a look at it please? After this we can merge.

melonora · 2024-04-04T13:26:23Z

Yeah will fix it when at the airport

melonora · 2024-04-04T14:05:35Z

hmm wait this PR was fine as the follow up PR the tests just passed. However with the last change on the follow up PR it is failing

melonora · 2024-04-04T14:23:12Z

Yeah as you can see just simply rerunning the failed job here makes it pass so here the issue was not a bug in the code.

timtreis

very few minor things

timtreis · 2024-04-04T14:35:51Z

 cax: PatchCollection,
 fig_params: FigParams,
- value_to_plot: str | None,
+ value_to_plot: str | None, # str | None,


timtreis · 2024-04-04T14:37:44Z

- element_table_mapping[element].add(table_name)
+ if isinstance(mapping := element_table_mapping[element], set):
+ mapping.add(table_name)
+ assert isinstance(element_table_mapping, dict)


I personally dislike spurious asserts. Can you verify before and block this path preemptively? If not, rather throw an informative warning

This is purely due to mypy not accepting anything else. The only way to get around this is by adopting the whole code base to use params per element.

Mypy gets very confused because of the data structure constantly changing. I tried casting but in most cases this does not help

timtreis · 2024-04-04T14:38:06Z

+ params.element_table_mapping
+ )
+
+ assert isinstance(element_table_mapping, dict)


timtreis · 2024-04-04T14:38:38Z

+ params.element_table_mapping
+ )
+
+ assert isinstance(element_table_mapping, dict)


timtreis · 2024-04-04T14:42:44Z

 else:
- if len(element_table_mapping[element_name].copy()) != 0:
- for table_name in element_table_mapping[element_name].copy():
+ if isinstance(mapping := element_table_mapping[element_name], set) and len(mapping.copy()) != 0:


reads a bit code-golf-y 👀 Also, why are you copying? Theoretically, you could also remove the != 0 since the len of an empty set is also 0 (False)

again partially mypy as for the copy, if you remove from a set during a for loop you will get an error because indices are removed while looping over the object. Here the one in len is unnecessary though so removing that.

timtreis · 2024-04-04T14:43:23Z

- if len(element_table_mapping[element_name].copy()) != 0:
- for table_name in element_table_mapping[element_name].copy():
+ if isinstance(mapping := element_table_mapping[element_name], set) and len(mapping.copy()) != 0:
+ for table_name in mapping.copy():


These are shallow copies no? Why do you do that?

timtreis · 2024-04-04T14:43:54Z

 params.col_for_color = [None] * len(render_elements)
 else:
+ assert isinstance(params.color, list)
+ assert isinstance(params.col_for_color, list)


melonora · 2024-04-04T15:50:37Z

Merging after talking with @timtreis

melonora · 2024-04-01T09:32:27Z

closes#234
Follow up on #235 fixing the plotting color columns that are numerical. First this fixes the visibility of the colorbar when plotting labels with a color. The issue here was that due to the outline_alpha being set to 0 when outline is False and outline of the label being plotted after the labels it would overwrite the colorbar. This was fixed by first plotting outline and then the label. A potential issue here is that when both alpha and outline alpha are 1 the outline will not be visible, but currently I thought this best as there is no clear way to decouple the colorbar from the outline. The general rule here is whatever is plotted last is visible.

The other issue got reported by @LucaMarconato in #234 . Ultimately, this was an issue when plotting feature0 in the models2 notebook in spatialdata-notebooks. The issue here was that 3 shapes are being plotted, one multipolygon and 2 polygons. This results in a color_vector of length 3 which is convertible to a RGB color, causing 1 color to be created for 3 shapes. This PR fixes the issue by checking whether we are dealing with a 1 dimensional color vector of length 3 or 4 and whether it then has the same length as the number of shapes.

The plot in models2 would now look like this:

codecov-commenter · 2024-04-01T10:02:33Z

Codecov Report

❌ Patch coverage is 50.00000% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.70%. Comparing base (9ddebfe) to head (46448b9).
⚠️ Report is 344 commits behind head on main.

Files with missing lines	Patch %	Lines
src/spatialdata_plot/pl/utils.py	38.46%	8 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #236 +/- ##
==========================================
- Coverage 78.01% 77.70% -0.31% 
==========================================
Files 11 11 Lines 1710 1718 +8 ==========================================
+ Hits 1334 1335 +1 - Misses 376 383 +7

Files with missing lines	Coverage Δ
src/spatialdata_plot/pl/render.py	`94.61% <100.00%> (ø)`
src/spatialdata_plot/pl/utils.py	`66.66% <38.46%> (-0.47%)`	⬇️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

melonora · 2024-04-01T12:58:07Z

lovely, tests in #237 do pass so we can ignore the failing test here.

giovp

ok for me ~~but maybe a very small test that would have caught this?~~ ah nvm ok with #237

LucaMarconato · 2024-04-04T13:24:37Z

Thanks @melonora for the PR! I noticed that neither main neither this PR is able to run the technology_mibitof.ipynb notebook because of a plot of a categorical into labels that fails.

I see a similar test is failing in this PR, so probably worth fixing them both, could you have a look at it please? After this we can merge.

melonora · 2024-04-04T13:26:23Z

Yeah will fix it when at the airport

melonora · 2024-04-04T14:05:35Z

hmm wait this PR was fine as the follow up PR the tests just passed. However with the last change on the follow up PR it is failing

melonora · 2024-04-04T14:23:12Z

Yeah as you can see just simply rerunning the failed job here makes it pass so here the issue was not a bug in the code.

timtreis

very few minor things

timtreis · 2024-04-04T14:35:51Z

 cax: PatchCollection,
 fig_params: FigParams,
- value_to_plot: str | None,
+ value_to_plot: str | None, # str | None,


timtreis · 2024-04-04T14:37:44Z

- element_table_mapping[element].add(table_name)
+ if isinstance(mapping := element_table_mapping[element], set):
+ mapping.add(table_name)
+ assert isinstance(element_table_mapping, dict)


I personally dislike spurious asserts. Can you verify before and block this path preemptively? If not, rather throw an informative warning

This is purely due to mypy not accepting anything else. The only way to get around this is by adopting the whole code base to use params per element.

Mypy gets very confused because of the data structure constantly changing. I tried casting but in most cases this does not help

timtreis · 2024-04-04T14:38:06Z

+ params.element_table_mapping
+ )
+
+ assert isinstance(element_table_mapping, dict)


timtreis · 2024-04-04T14:38:38Z

+ params.element_table_mapping
+ )
+
+ assert isinstance(element_table_mapping, dict)


timtreis · 2024-04-04T14:42:44Z

 else:
- if len(element_table_mapping[element_name].copy()) != 0:
- for table_name in element_table_mapping[element_name].copy():
+ if isinstance(mapping := element_table_mapping[element_name], set) and len(mapping.copy()) != 0:


reads a bit code-golf-y 👀 Also, why are you copying? Theoretically, you could also remove the != 0 since the len of an empty set is also 0 (False)

again partially mypy as for the copy, if you remove from a set during a for loop you will get an error because indices are removed while looping over the object. Here the one in len is unnecessary though so removing that.

timtreis · 2024-04-04T14:43:23Z

- if len(element_table_mapping[element_name].copy()) != 0:
- for table_name in element_table_mapping[element_name].copy():
+ if isinstance(mapping := element_table_mapping[element_name], set) and len(mapping.copy()) != 0:
+ for table_name in mapping.copy():


These are shallow copies no? Why do you do that?

timtreis · 2024-04-04T14:43:54Z

 params.col_for_color = [None] * len(render_elements)
 else:
+ assert isinstance(params.color, list)
+ assert isinstance(params.col_for_color, list)


melonora · 2024-04-04T15:50:37Z

Merging after talking with @timtreis

melonora · 2024-04-01T09:32:27Z

closes#234
Follow up on #235 fixing the plotting color columns that are numerical. First this fixes the visibility of the colorbar when plotting labels with a color. The issue here was that due to the outline_alpha being set to 0 when outline is False and outline of the label being plotted after the labels it would overwrite the colorbar. This was fixed by first plotting outline and then the label. A potential issue here is that when both alpha and outline alpha are 1 the outline will not be visible, but currently I thought this best as there is no clear way to decouple the colorbar from the outline. The general rule here is whatever is plotted last is visible.

The other issue got reported by @LucaMarconato in #234 . Ultimately, this was an issue when plotting feature0 in the models2 notebook in spatialdata-notebooks. The issue here was that 3 shapes are being plotted, one multipolygon and 2 polygons. This results in a color_vector of length 3 which is convertible to a RGB color, causing 1 color to be created for 3 shapes. This PR fixes the issue by checking whether we are dealing with a 1 dimensional color vector of length 3 or 4 and whether it then has the same length as the number of shapes.

The plot in models2 would now look like this:

codecov-commenter · 2024-04-01T10:02:33Z

Codecov Report

❌ Patch coverage is 50.00000% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.70%. Comparing base (9ddebfe) to head (46448b9).
⚠️ Report is 344 commits behind head on main.

Files with missing lines	Patch %	Lines
src/spatialdata_plot/pl/utils.py	38.46%	8 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #236 +/- ##
==========================================
- Coverage 78.01% 77.70% -0.31% 
==========================================
Files 11 11 Lines 1710 1718 +8 ==========================================
+ Hits 1334 1335 +1 - Misses 376 383 +7

Files with missing lines	Coverage Δ
src/spatialdata_plot/pl/render.py	`94.61% <100.00%> (ø)`
src/spatialdata_plot/pl/utils.py	`66.66% <38.46%> (-0.47%)`	⬇️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

melonora · 2024-04-01T12:58:07Z

lovely, tests in #237 do pass so we can ignore the failing test here.

giovp

ok for me ~~but maybe a very small test that would have caught this?~~ ah nvm ok with #237

LucaMarconato · 2024-04-04T13:24:37Z

Thanks @melonora for the PR! I noticed that neither main neither this PR is able to run the technology_mibitof.ipynb notebook because of a plot of a categorical into labels that fails.

I see a similar test is failing in this PR, so probably worth fixing them both, could you have a look at it please? After this we can merge.

melonora · 2024-04-04T13:26:23Z

Yeah will fix it when at the airport

melonora · 2024-04-04T14:05:35Z

hmm wait this PR was fine as the follow up PR the tests just passed. However with the last change on the follow up PR it is failing

melonora · 2024-04-04T14:23:12Z

Yeah as you can see just simply rerunning the failed job here makes it pass so here the issue was not a bug in the code.

timtreis

very few minor things

timtreis · 2024-04-04T14:35:51Z

 cax: PatchCollection,
 fig_params: FigParams,
- value_to_plot: str | None,
+ value_to_plot: str | None, # str | None,


timtreis · 2024-04-04T14:37:44Z

- element_table_mapping[element].add(table_name)
+ if isinstance(mapping := element_table_mapping[element], set):
+ mapping.add(table_name)
+ assert isinstance(element_table_mapping, dict)


I personally dislike spurious asserts. Can you verify before and block this path preemptively? If not, rather throw an informative warning

This is purely due to mypy not accepting anything else. The only way to get around this is by adopting the whole code base to use params per element.

Mypy gets very confused because of the data structure constantly changing. I tried casting but in most cases this does not help

timtreis · 2024-04-04T14:38:06Z

+ params.element_table_mapping
+ )
+
+ assert isinstance(element_table_mapping, dict)


timtreis · 2024-04-04T14:38:38Z

+ params.element_table_mapping
+ )
+
+ assert isinstance(element_table_mapping, dict)


timtreis · 2024-04-04T14:42:44Z

 else:
- if len(element_table_mapping[element_name].copy()) != 0:
- for table_name in element_table_mapping[element_name].copy():
+ if isinstance(mapping := element_table_mapping[element_name], set) and len(mapping.copy()) != 0:


reads a bit code-golf-y 👀 Also, why are you copying? Theoretically, you could also remove the != 0 since the len of an empty set is also 0 (False)

again partially mypy as for the copy, if you remove from a set during a for loop you will get an error because indices are removed while looping over the object. Here the one in len is unnecessary though so removing that.

timtreis · 2024-04-04T14:43:23Z

- if len(element_table_mapping[element_name].copy()) != 0:
- for table_name in element_table_mapping[element_name].copy():
+ if isinstance(mapping := element_table_mapping[element_name], set) and len(mapping.copy()) != 0:
+ for table_name in mapping.copy():


These are shallow copies no? Why do you do that?

timtreis · 2024-04-04T14:43:54Z

 params.col_for_color = [None] * len(render_elements)
 else:
+ assert isinstance(params.color, list)
+ assert isinstance(params.col_for_color, list)


melonora · 2024-04-04T15:50:37Z

Merging after talking with @timtreis

melonora · 2024-04-01T09:32:27Z

closes#234
Follow up on #235 fixing the plotting color columns that are numerical. First this fixes the visibility of the colorbar when plotting labels with a color. The issue here was that due to the outline_alpha being set to 0 when outline is False and outline of the label being plotted after the labels it would overwrite the colorbar. This was fixed by first plotting outline and then the label. A potential issue here is that when both alpha and outline alpha are 1 the outline will not be visible, but currently I thought this best as there is no clear way to decouple the colorbar from the outline. The general rule here is whatever is plotted last is visible.

The other issue got reported by @LucaMarconato in #234 . Ultimately, this was an issue when plotting feature0 in the models2 notebook in spatialdata-notebooks. The issue here was that 3 shapes are being plotted, one multipolygon and 2 polygons. This results in a color_vector of length 3 which is convertible to a RGB color, causing 1 color to be created for 3 shapes. This PR fixes the issue by checking whether we are dealing with a 1 dimensional color vector of length 3 or 4 and whether it then has the same length as the number of shapes.

The plot in models2 would now look like this:

codecov-commenter · 2024-04-01T10:02:33Z

Codecov Report

❌ Patch coverage is 50.00000% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.70%. Comparing base (9ddebfe) to head (46448b9).
⚠️ Report is 344 commits behind head on main.

Files with missing lines	Patch %	Lines
src/spatialdata_plot/pl/utils.py	38.46%	8 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #236 +/- ##
==========================================
- Coverage 78.01% 77.70% -0.31% 
==========================================
Files 11 11 Lines 1710 1718 +8 ==========================================
+ Hits 1334 1335 +1 - Misses 376 383 +7

Files with missing lines	Coverage Δ
src/spatialdata_plot/pl/render.py	`94.61% <100.00%> (ø)`
src/spatialdata_plot/pl/utils.py	`66.66% <38.46%> (-0.47%)`	⬇️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

melonora · 2024-04-01T12:58:07Z

lovely, tests in #237 do pass so we can ignore the failing test here.

giovp

ok for me ~~but maybe a very small test that would have caught this?~~ ah nvm ok with #237

LucaMarconato · 2024-04-04T13:24:37Z

Thanks @melonora for the PR! I noticed that neither main neither this PR is able to run the technology_mibitof.ipynb notebook because of a plot of a categorical into labels that fails.

I see a similar test is failing in this PR, so probably worth fixing them both, could you have a look at it please? After this we can merge.

melonora · 2024-04-04T13:26:23Z

Yeah will fix it when at the airport

melonora · 2024-04-04T14:05:35Z

hmm wait this PR was fine as the follow up PR the tests just passed. However with the last change on the follow up PR it is failing

melonora · 2024-04-04T14:23:12Z

Yeah as you can see just simply rerunning the failed job here makes it pass so here the issue was not a bug in the code.

timtreis

very few minor things

timtreis · 2024-04-04T14:35:51Z

 cax: PatchCollection,
 fig_params: FigParams,
- value_to_plot: str | None,
+ value_to_plot: str | None, # str | None,


timtreis · 2024-04-04T14:37:44Z

- element_table_mapping[element].add(table_name)
+ if isinstance(mapping := element_table_mapping[element], set):
+ mapping.add(table_name)
+ assert isinstance(element_table_mapping, dict)


I personally dislike spurious asserts. Can you verify before and block this path preemptively? If not, rather throw an informative warning

This is purely due to mypy not accepting anything else. The only way to get around this is by adopting the whole code base to use params per element.

Mypy gets very confused because of the data structure constantly changing. I tried casting but in most cases this does not help

timtreis · 2024-04-04T14:38:06Z

+ params.element_table_mapping
+ )
+
+ assert isinstance(element_table_mapping, dict)


timtreis · 2024-04-04T14:38:38Z

+ params.element_table_mapping
+ )
+
+ assert isinstance(element_table_mapping, dict)


timtreis · 2024-04-04T14:42:44Z

 else:
- if len(element_table_mapping[element_name].copy()) != 0:
- for table_name in element_table_mapping[element_name].copy():
+ if isinstance(mapping := element_table_mapping[element_name], set) and len(mapping.copy()) != 0:


reads a bit code-golf-y 👀 Also, why are you copying? Theoretically, you could also remove the != 0 since the len of an empty set is also 0 (False)

again partially mypy as for the copy, if you remove from a set during a for loop you will get an error because indices are removed while looping over the object. Here the one in len is unnecessary though so removing that.

timtreis · 2024-04-04T14:43:23Z

- if len(element_table_mapping[element_name].copy()) != 0:
- for table_name in element_table_mapping[element_name].copy():
+ if isinstance(mapping := element_table_mapping[element_name], set) and len(mapping.copy()) != 0:
+ for table_name in mapping.copy():


These are shallow copies no? Why do you do that?

timtreis · 2024-04-04T14:43:54Z

 params.col_for_color = [None] * len(render_elements)
 else:
+ assert isinstance(params.color, list)
+ assert isinstance(params.col_for_color, list)


melonora · 2024-04-04T15:50:37Z

Merging after talking with @timtreis

melonora · 2024-04-01T09:32:27Z

closes#234
Follow up on #235 fixing the plotting color columns that are numerical. First this fixes the visibility of the colorbar when plotting labels with a color. The issue here was that due to the outline_alpha being set to 0 when outline is False and outline of the label being plotted after the labels it would overwrite the colorbar. This was fixed by first plotting outline and then the label. A potential issue here is that when both alpha and outline alpha are 1 the outline will not be visible, but currently I thought this best as there is no clear way to decouple the colorbar from the outline. The general rule here is whatever is plotted last is visible.

The other issue got reported by @LucaMarconato in #234 . Ultimately, this was an issue when plotting feature0 in the models2 notebook in spatialdata-notebooks. The issue here was that 3 shapes are being plotted, one multipolygon and 2 polygons. This results in a color_vector of length 3 which is convertible to a RGB color, causing 1 color to be created for 3 shapes. This PR fixes the issue by checking whether we are dealing with a 1 dimensional color vector of length 3 or 4 and whether it then has the same length as the number of shapes.

The plot in models2 would now look like this:

codecov-commenter · 2024-04-01T10:02:33Z

Codecov Report

❌ Patch coverage is 50.00000% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 77.70%. Comparing base (9ddebfe) to head (46448b9).
⚠️ Report is 344 commits behind head on main.

Files with missing lines	Patch %	Lines
src/spatialdata_plot/pl/utils.py	38.46%	8 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #236 +/- ##
==========================================
- Coverage 78.01% 77.70% -0.31% 
==========================================
Files 11 11 Lines 1710 1718 +8 ==========================================
+ Hits 1334 1335 +1 - Misses 376 383 +7

Files with missing lines	Coverage Δ
src/spatialdata_plot/pl/render.py	`94.61% <100.00%> (ø)`
src/spatialdata_plot/pl/utils.py	`66.66% <38.46%> (-0.47%)`	⬇️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

melonora · 2024-04-01T12:58:07Z

lovely, tests in #237 do pass so we can ignore the failing test here.

giovp

ok for me ~~but maybe a very small test that would have caught this?~~ ah nvm ok with #237

LucaMarconato · 2024-04-04T13:24:37Z

Thanks @melonora for the PR! I noticed that neither main neither this PR is able to run the technology_mibitof.ipynb notebook because of a plot of a categorical into labels that fails.

I see a similar test is failing in this PR, so probably worth fixing them both, could you have a look at it please? After this we can merge.

melonora · 2024-04-04T13:26:23Z

Yeah will fix it when at the airport

melonora · 2024-04-04T14:05:35Z

hmm wait this PR was fine as the follow up PR the tests just passed. However with the last change on the follow up PR it is failing

melonora · 2024-04-04T14:23:12Z

Yeah as you can see just simply rerunning the failed job here makes it pass so here the issue was not a bug in the code.

timtreis

very few minor things

timtreis · 2024-04-04T14:35:51Z

 cax: PatchCollection,
 fig_params: FigParams,
- value_to_plot: str | None,
+ value_to_plot: str | None, # str | None,


timtreis · 2024-04-04T14:37:44Z

- element_table_mapping[element].add(table_name)
+ if isinstance(mapping := element_table_mapping[element], set):
+ mapping.add(table_name)
+ assert isinstance(element_table_mapping, dict)


I personally dislike spurious asserts. Can you verify before and block this path preemptively? If not, rather throw an informative warning

This is purely due to mypy not accepting anything else. The only way to get around this is by adopting the whole code base to use params per element.

Mypy gets very confused because of the data structure constantly changing. I tried casting but in most cases this does not help

timtreis · 2024-04-04T14:38:06Z

+ params.element_table_mapping
+ )
+
+ assert isinstance(element_table_mapping, dict)


timtreis · 2024-04-04T14:38:38Z

+ params.element_table_mapping
+ )
+
+ assert isinstance(element_table_mapping, dict)


timtreis · 2024-04-04T14:42:44Z

 else:
- if len(element_table_mapping[element_name].copy()) != 0:
- for table_name in element_table_mapping[element_name].copy():
+ if isinstance(mapping := element_table_mapping[element_name], set) and len(mapping.copy()) != 0:


reads a bit code-golf-y 👀 Also, why are you copying? Theoretically, you could also remove the != 0 since the len of an empty set is also 0 (False)

again partially mypy as for the copy, if you remove from a set during a for loop you will get an error because indices are removed while looping over the object. Here the one in len is unnecessary though so removing that.

timtreis · 2024-04-04T14:43:23Z

- if len(element_table_mapping[element_name].copy()) != 0:
- for table_name in element_table_mapping[element_name].copy():
+ if isinstance(mapping := element_table_mapping[element_name], set) and len(mapping.copy()) != 0:
+ for table_name in mapping.copy():


These are shallow copies no? Why do you do that?

timtreis · 2024-04-04T14:43:54Z

 params.col_for_color = [None] * len(render_elements)
 else:
+ assert isinstance(params.color, list)
+ assert isinstance(params.col_for_color, list)


melonora · 2024-04-04T15:50:37Z

Merging after talking with @timtreis

melonora added 3 commits April 1, 2024 00:54

fix var colorbar labels
f0452d4

fix var plotting in case of vector length 3/4
6f63830

fix tests
9445e36

melonora mentioned this pull request Apr 1, 2024
Pass docs #237
Merged

giovp approved these changes Apr 3, 2024
View reviewed changes

timtreis requested changes Apr 4, 2024
View reviewed changes

melonora added 5 commits April 4, 2024 17:04

address comments
3d68be0

remove unnecessary check
5793bf2

more informative name
c33f84a

fix prettier
56ed753

Merge branch 'main' into fix_var_plotting
46448b9

melonora merged commit cbc0935 into scverse:mainApr 4, 2024

melonora added 3 commits April 1, 2024 00:54

fix var colorbar labels
f0452d4

fix var plotting in case of vector length 3/4
6f63830

fix tests
9445e36

melonora mentioned this pull request Apr 1, 2024
Pass docs #237
Merged

giovp approved these changes Apr 3, 2024
View reviewed changes

timtreis requested changes Apr 4, 2024
View reviewed changes

melonora added 5 commits April 4, 2024 17:04

address comments
3d68be0

remove unnecessary check
5793bf2

more informative name
c33f84a

fix prettier
56ed753

Merge branch 'main' into fix_var_plotting
46448b9

melonora merged commit cbc0935 into scverse:mainApr 4, 2024

melonora added 3 commits April 1, 2024 00:54

fix var colorbar labels
f0452d4

fix var plotting in case of vector length 3/4
6f63830

fix tests
9445e36

melonora mentioned this pull request Apr 1, 2024
Pass docs #237
Merged

giovp approved these changes Apr 3, 2024
View reviewed changes

timtreis requested changes Apr 4, 2024
View reviewed changes

melonora added 5 commits April 4, 2024 17:04

address comments
3d68be0

remove unnecessary check
5793bf2

more informative name
c33f84a

fix prettier
56ed753

Merge branch 'main' into fix_var_plotting
46448b9

melonora merged commit cbc0935 into scverse:mainApr 4, 2024

melonora added 3 commits April 1, 2024 00:54

fix var colorbar labels
f0452d4

fix var plotting in case of vector length 3/4
6f63830

fix tests
9445e36

melonora mentioned this pull request Apr 1, 2024
Pass docs #237
Merged

giovp approved these changes Apr 3, 2024
View reviewed changes

timtreis requested changes Apr 4, 2024
View reviewed changes

melonora added 5 commits April 4, 2024 17:04

address comments
3d68be0

remove unnecessary check
5793bf2

more informative name
c33f84a

fix prettier
56ed753

Merge branch 'main' into fix_var_plotting
46448b9

melonora merged commit cbc0935 into scverse:mainApr 4, 2024

melonora added 3 commits April 1, 2024 00:54

fix var colorbar labels
f0452d4

fix var plotting in case of vector length 3/4
6f63830

fix tests
9445e36

melonora mentioned this pull request Apr 1, 2024
Pass docs #237
Merged

giovp approved these changes Apr 3, 2024
View reviewed changes

timtreis requested changes Apr 4, 2024
View reviewed changes

melonora added 5 commits April 4, 2024 17:04

address comments
3d68be0

remove unnecessary check
5793bf2

more informative name
c33f84a

fix prettier
56ed753

Merge branch 'main' into fix_var_plotting
46448b9

melonora merged commit cbc0935 into scverse:mainApr 4, 2024

melonora added 3 commits April 1, 2024 00:54

fix var colorbar labels
f0452d4

fix var plotting in case of vector length 3/4
6f63830

fix tests
9445e36

melonora mentioned this pull request Apr 1, 2024
Pass docs #237
Merged

giovp approved these changes Apr 3, 2024
View reviewed changes

timtreis requested changes Apr 4, 2024
View reviewed changes

melonora added 5 commits April 4, 2024 17:04

address comments
3d68be0

remove unnecessary check
5793bf2

more informative name
c33f84a

fix prettier
56ed753

Merge branch 'main' into fix_var_plotting
46448b9

melonora merged commit cbc0935 into scverse:mainApr 4, 2024

melonora added 3 commits April 1, 2024 00:54

fix var colorbar labels
f0452d4

fix var plotting in case of vector length 3/4
6f63830

fix tests
9445e36

melonora mentioned this pull request Apr 1, 2024
Pass docs #237
Merged

giovp approved these changes Apr 3, 2024
View reviewed changes

timtreis requested changes Apr 4, 2024
View reviewed changes

melonora added 5 commits April 4, 2024 17:04

address comments
3d68be0

remove unnecessary check
5793bf2

more informative name
c33f84a

fix prettier
56ed753

Merge branch 'main' into fix_var_plotting
46448b9

melonora merged commit cbc0935 into scverse:mainApr 4, 2024

melonora added 3 commits April 1, 2024 00:54

fix var colorbar labels
f0452d4

fix var plotting in case of vector length 3/4
6f63830

fix tests
9445e36

melonora mentioned this pull request Apr 1, 2024
Pass docs #237
Merged

giovp approved these changes Apr 3, 2024
View reviewed changes

timtreis requested changes Apr 4, 2024
View reviewed changes

melonora added 5 commits April 4, 2024 17:04

address comments
3d68be0

remove unnecessary check
5793bf2

more informative name
c33f84a

fix prettier
56ed753

Merge branch 'main' into fix_var_plotting
46448b9

melonora merged commit cbc0935 into scverse:mainApr 4, 2024

Uh oh!

Conversation

melonora commented Apr 1, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Apr 1, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

melonora commented Apr 1, 2024

Uh oh!

giovp left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LucaMarconato commented Apr 4, 2024

Uh oh!

melonora commented Apr 4, 2024

Uh oh!

melonora commented Apr 4, 2024

Uh oh!

melonora commented Apr 4, 2024

Uh oh!

timtreis left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

melonora commented Apr 4, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Uh oh!

Conversation

melonora commented Apr 1, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Apr 1, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

melonora commented Apr 1, 2024

Uh oh!

giovp left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LucaMarconato commented Apr 4, 2024

Uh oh!

melonora commented Apr 4, 2024

Uh oh!

melonora commented Apr 4, 2024

Uh oh!

melonora commented Apr 1, 2024•
edited
Loading

codecov-commenter commented Apr 1, 2024•
edited
Loading

giovp left a comment •
edited
Loading

melonora commented Apr 1, 2024•
edited
Loading

codecov-commenter commented Apr 1, 2024•
edited
Loading

giovp left a comment •
edited
Loading