-
-
Notifications
You must be signed in to change notification settings - Fork 313
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
Use separate reference images per backend #3520
Conversation
Compile Times benchmarkNote, that these numbers may fluctuate on the CI servers, so take them with a grain of salt. All benchmark results are based on the mean time and negative percent mean faster than the base branch. Note, that GLMakie + WGLMakie run on an emulated GPU, so the runtime benchmark is much slower. Results are from running: using_time = @ctime using Backend
# Compile time
create_time = @ctime fig = scatter(1:4; color=1:4, colormap=:turbo, markersize=20, visible=true)
display_time = @ctime Makie.colorbuffer(display(fig))
# Runtime
create_time = @benchmark fig = scatter(1:4; color=1:4, colormap=:turbo, markersize=20, visible=true)
display_time = @benchmark Makie.colorbuffer(fig)
|
This PR works now, what remains is to update the 17 missing images using ReferenceUpdater and to adjust the |
@DanielVandH this PR now only fails with a triplot reference, but only on 1.6. I can't see the generated image right now because those artifacts aren't saved but this must be due to RNG differences I think. Any idea what could be going on? |
@jkrumbiegel it would have to be due to an RNG issue if anything. Were any new tests inserted in between the existing triangulation tests since they were added? That would disrupt it. |
@DanielVandH I don't think so, no. The tests work fine on Julia 1 but not on 1.6. But the point of this PR was to increase the test sensitivity so it's totally possible the 1.6 version was always broken, but just flew under the radar. Aren't we using stable rng so that we don't have the problem of different random numbers in different julia versions? Maybe there's one aspect of that plotting function that doesn't fully comply with that, at least currently I have no other hypothesis. |
Is this always the case? It's possible that the RNG only matters for a few triangles and so isn't always seen (probably not, though). If there were no other uses of the |
I've just pushed a change so that 1.6 also stores its reference images as an artifact again, I thought we didn't need it and was immediately proven wrong :D |
Ok here we go @DanielVandH, reference on 1 and recorded on 1.6 There are differences at the top border |
Weird.. there are more points being inserted onto the boundary. That means the RNG for the mesh refinement is not working or changed somehow. I can try and take a look, but I haven't really had time for any coding this past month so I'm not sure when I can take a proper look. This test is not too crucial though, other tests also use a custom bounding box / holes which seems to be what this test was for, in case you want to just remove it. |
Ok I'll temporarily disable it then, thanks! |
The git repo approach is a bit complex to set up, and it has the additional issue that you cannot update or see images that haven't failed. So for now I'm putting that on hold.
So in this PR, each backend creates its own subfolder for reference images. The special glmakie tests are just merged into a single set with the general tests by allowing to specify more than one set of reference tests at once:
Therefore, there is just one set of refimages left called
reference_images.tar
. I could already upload this tar file to the 0.20 release for testing of this PR.Because we don't fail if there are reference images for which no tests exist, it should not matter to each individual backend that the reference image tar contains data for all backends at once. We can merge the three recorded folders into one after all backend tests have run, this artifact could then be used to later rewrite the reference updater to be able to update all three backends at once.