Skip to content
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

Fix transformed labels with outline #275

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

aeisenbarth
Copy link
Contributor

@aeisenbarth aeisenbarth 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:

@aeisenbarth aeisenbarth force-pushed the fix-transformed-labels-outline branch from 6fb8c55 to b63030d Compare June 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
Contributor Author

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
    def test_plot_can_render_contour_with_infill(self, sdata_blobs):
        # Rendering with fill_alpha == outline_alpha takes a special code path
        sdata_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.

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

timtreis commented Sep 3, 2024

@aeisenbarth can you rename that one function?

image

@timtreis timtreis self-assigned this Oct 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working labels 🏷️ Anything related to Labels priority: medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Label outlines cause wrong transformation
4 participants