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 tests from modified get_element_instances #287

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

giovp
Copy link
Member

@giovp giovp commented Jul 9, 2024

folllow scverse/spatialdata#621 and should be merged after that.

I modified couple of plots that I think it made sense, but for two, specifically the NotebookTransformation ones for rotation and affine I did not, you can see below the new version (left) v. old version (right)

affine
image

rotation
image

you can see that the only thing that really changes is the background color, I wonder if it is a matplolib version problem, as I don't think spatialdata#621 impacts that. Any idea @melonora @timtreis ?

@melonora
Copy link
Contributor

melonora commented Jul 9, 2024

yeah this is a known issue, matplotlib is producing different results dependent on both version and platform.

@giovp
Copy link
Member Author

giovp commented Jul 9, 2024

ok so the tests should pass for those two 🤞

@melonora
Copy link
Contributor

melonora commented Jul 9, 2024

hmm this label_categorical color was wrong and is still wrong. Given that it was broken I am ok with this PR, but we need to fix it. What is indicated as being C is actually background label. @timtreis I vaguely remember you workin on a fix for this. Am I correct?

@giovp
Copy link
Member Author

giovp commented Jul 9, 2024

I'm afraid the tests failing are the ones of the figures I have added, I wonder if the reason is precisely this version differing behaviour. Increase tolerance ? or someone has an ubuntu machine to recreate figures?

@melonora
Copy link
Contributor

melonora commented Jul 9, 2024

Increasing tolerance will not help as this does not account for large difference in colors which happens with different color for the labels or background.

@timtreis
Copy link
Member

timtreis commented Jul 9, 2024

Yeah, no idea why this is happening. Noticed it in #259 originally but I still have no idea what's causing it. Local to me it looks fine. For this case, I'm just using the image of the runner itself.

@giovp
Copy link
Member Author

giovp commented Jul 10, 2024

Good point, uploaded the new ones from the runner artifacts

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.48%. Comparing base (b935933) to head (ca9f0f6).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #287   +/-   ##
=======================================
  Coverage   79.48%   79.48%           
=======================================
  Files          11       11           
  Lines        1628     1628           
=======================================
  Hits         1294     1294           
  Misses        334      334           

@giovp
Copy link
Member Author

giovp commented Jul 10, 2024

wtf how can 3.9 and 3.10 not be consistent?

@melonora
Copy link
Contributor

Because matplotlib. Had this before, in these cases for now if this happens we accept the PR

@giovp
Copy link
Member Author

giovp commented Jul 10, 2024

mmh but this is a bit suspicious, like the results seems to put labels with different orders, despite everything being generated by RNG. In the last commit, I took artefacts from 3.9 and copied it to 3.10 and still now both fails

@timtreis timtreis self-assigned this Jul 10, 2024
@melonora
Copy link
Contributor

@timtreis are you still planning to pick this up? Otherwise I can.

@timtreis
Copy link
Member

I think I tightened the GH actions enough so that 3.9 and 3.10 are now consistent. At least I didn't run into inconsistencies in the few last PRs. We still get the black bg on some of the raccoons, only on the runner though.

Can this be closed?

@LucaMarconato
Copy link
Member

I checked the code and the one from Giovanni is correct. The max of the labels is 5, but the labels are non-contiguous, so using len(get_element_instances()) is the way to go.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants