Skip to content

Conversation

@aeisenbarth
Copy link
Contributor

@aeisenbarthaeisenbarth commented Jun 12, 2024

Outlines are rendered on an axis separate from the filled labels. The transformation was only applied to the filled labels, so that the outlines had an identity transform. This was not observable when outlines were off or the transform was an identity (as in the blobs data).

Closes#273

For comparison with the example in the linked issue:

@aeisenbarthaeisenbarthforce-pushed the fix-transformed-labels-outline branch from 6fb8c55 to b63030dCompareJune 13, 2024 12:08
@codecov-commenter
Copy link

codecov-commenter commented Jun 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.27%. Comparing base (ffd4a1f) to head (61ff47c).

Additional details and impacted files
@@ Coverage Diff @@## main #275 +/- ## ======================================= Coverage 84.27% 84.27% ======================================= Files 8 8 Lines 1558 1558 ======================================= Hits 1313 1313 Misses 245 245 

@aeisenbarth
Copy link
ContributorAuthor

aeisenbarth commented Jun 13, 2024

I looked into the 2 lines of missing coverage. These are the two lines that seemed to be wrongly indented. By indenting them, I moved them into an untested code block, so that they are not covered anymore.

The else-branch is commented as "Default: no alpha, contour = infill" and not covered by tests. I tried adding a test to improve coverage, but am not convinced that this is the correct result. I think the code is wrong, the labels should be filled and my test (below) should fail. That means before adding the test we need a plot file of the correct, expected result. Or the else-branch should be removed.
image

Test case for infill
deftest_plot_can_render_contour_with_infill(self, sdata_blobs): # Rendering with fill_alpha == outline_alpha takes a special code pathsdata_blobs.pl.render_labels( "blobs_labels", color="channel_0_sum", fill_alpha=1.0, outline=True, outline_alpha=1.0, ).pl.show()

@melonora
Copy link
Contributor

Hi @aeisenbarth, thanks for the PR. I will merge a couple of PRs first and then adjust this one if required.

@timtreistimtreis added labels 🏷️ Anything related to Labels bug Something isn't working priority: medium labels Jul 14, 2024
@aeisenbarthaeisenbarthforce-pushed the fix-transformed-labels-outline branch from 2dd8444 to 61ff47cCompareSeptember 1, 2024 08:46
@timtreis
Copy link
Member

@aeisenbarth can you rename that one function?

image

@timtreistimtreis self-assigned this Oct 23, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bugSomething isn't workinglabels 🏷️Anything related to Labelspriority: medium

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Label outlines cause wrong transformation

4 participants

@aeisenbarth@codecov-commenter@melonora@timtreis