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

Make triangulations unique to fix refimages issues. Also tweak examples used for refimages #4044

Merged
merged 8 commits into from
Aug 10, 2024

Conversation

DanielVandH
Copy link
Contributor

Description

Passes randomise=false to all calls to triangulate to force them to be unique.

I have also tweaked the refimage test examples so that issues with randomisation or iteration order across Julia versions hopefully won't break them anymore. This hopefully fixes the issues you were having @SimonDanisch

Type of change

  • Bug fix (non-breaking change which fixes an issue) (kinda)

Checklist

  • Added an entry in CHANGELOG.md (for new features and breaking changes)

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Anshul Singhvi <anshulsinghvi@gmail.com>
@DanielVandH
Copy link
Contributor Author

Are the tests failing because the refimages aren't updated yet, or they actually just failing because I've done something wrong here?

@asinghvi17
Copy link
Member

Looks like the reference images need an update because of this PR's changes, which makes sense.

@SimonDanisch
Copy link
Member

Have you checked that all tests look correct? Then I could upload the delaunaytriangulation related reference images.

@DanielVandH
Copy link
Contributor Author

DanielVandH commented Aug 9, 2024

When I made the PR at the time everything was correct. I can double check again, I just wasn't sure if any of the other changes since had updated the refimages as I'm not sure how that's done on your end.

I'll check again soon and let you know once I've confirmed.

@ffreyer
Copy link
Collaborator

ffreyer commented Aug 9, 2024

refimgs are partially updated for #3958 atm so some things will fail on master

@DanielVandH
Copy link
Contributor Author

I've double checked and everything seems fine. I added another randomise=false just for a bit more protection. There is a failing test

CairoMakie/Lines from outside.png    

but I don't imagine that's related.

@ffreyer
Copy link
Collaborator

ffreyer commented Aug 9, 2024

Yea that's a failure due to the other pr. The pr fixes a problem with how CairoMakie handles color interpolation with clipped lines. FastPixel should also be failing because of the other pr.

@ffreyer
Copy link
Collaborator

ffreyer commented Aug 10, 2024

Two ref images changed in point density:

before after
Screenshot 2024-08-10 142903 Screenshot 2024-08-10 142854
Screenshot 2024-08-10 142918 Screenshot 2024-08-10 142910

This changed in terms of triangulation:

Screenshot 2024-08-10 143601

And here it seems the colormapping changed, but I guess this could also be a triangulation change?

Screenshot 2024-08-10 143548

The new images look consistent to me across backends and the ones I didn't mention should be the same as before. I think all of these are fine/desired changes. @DanielVandH If these still look good to you I'd be happy to merge this

@DanielVandH
Copy link
Contributor Author

Those all look as intended to me! Thanks.

@ffreyer ffreyer closed this Aug 10, 2024
@ffreyer ffreyer reopened this Aug 10, 2024
@ffreyer ffreyer merged commit c8a149d into MakieOrg:master Aug 10, 2024
26 of 34 checks passed
@DanielVandH DanielVandH deleted the fixdeltri branch August 10, 2024 16:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Merged
Development

Successfully merging this pull request may close these issues.

None yet

4 participants