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 contour boundingbox with NaNs #3210

Merged
merged 12 commits into from
Dec 24, 2023
Merged

Conversation

t-bltg
Copy link
Collaborator

@t-bltg t-bltg commented Sep 4, 2023

Description

Fixes discord issue (fix #3211):

let
    a = b = 1:3
    c = [0 1 2; 1 2 3; 4 5 6]
    ct1 = contour(a, b, c; levels = [1, 2, 3], labels = true)   # OK
    d = [0 1 2; 1 2 3; 4 5 NaN]
    ct2 = contour(a, b, d; levels = [1, 2, 3], labels = true)   # NOK
end

Fix a CairoMakie bug when glyphs positions contained a NaN, corrupting the Cairo drawing state / context.

Type of change

Delete options that do not apply:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • Added an entry in NEWS.md (for new features and breaking changes)
  • Added or changed relevant sections in the documentation
  • Added unit tests for new algorithms, conversion methods, etc.
  • Added reference image tests for new plotting functions, recipes, visual options, etc.

@t-bltg t-bltg changed the title fix bb with NaNs fix contour boundingbox with NaNs Sep 4, 2023
@t-bltg t-bltg force-pushed the cont_bug_nan branch 3 times, most recently from 8b050bf to 1279ed6 Compare September 5, 2023 09:05
@Lightup1
Copy link

Lightup1 commented Sep 6, 2023

Hi @t-bltg ,
I tried this branch.
There is no error message now, but there are also no contour lines now.

@Lightup1
Copy link

Lightup1 commented Sep 6, 2023

image

@jkrumbiegel
Copy link
Member

Which backend was this?

@Lightup1
Copy link

Lightup1 commented Sep 6, 2023

@jkrumbiegel CairoMakie

@t-bltg
Copy link
Collaborator Author

t-bltg commented Sep 6, 2023

Let me have a look this morning.

@jkrumbiegel
Copy link
Member

Could be a bug in CairoMakie, too, because there is one little line segment and afterwards nothing, also the axis lines are missing. That usually happens when Cairo silently corrupts the surface state, which can happen with NaNs sometimes.

@t-bltg t-bltg marked this pull request as draft September 6, 2023 08:05
@t-bltg
Copy link
Collaborator Author

t-bltg commented Sep 6, 2023

Let's reason first with NaN-less arrays: c = [0 1 2; 1 2 3; 4 5 6].

What is visually represented is reverse(c; dims=1):

3×3 Matrix{Float64}:
 4.0  5.0    6.0
 1.0  2.0    3.0
 0.0  1.0    2.0

So this is contourf(a, b, c; levels):
contourf

With GLMakie (or CairoMakie), contour(a, b, c; levels, labels=true):
glmakie. Something could probably be done to enhance the label position for 2.

Now with NaNs, d = [0 1 2; 1 2 3; 4 5 NaN], contourf(a, b, d; levels):
contourf-nan.

So I will expect at least the 1 and 2 levels lines with labels to appear (3 is more tricky because I use NaNs internally to mask contour segments hidden by the label).

It is also difficult to work with such small arrays implying short contour segments.

I'll try to rework the PR.

@Lightup1
Copy link

Lightup1 commented Sep 8, 2023

GLMakie works fine now (except missing 2 labels).
CairoMakie produces no contour lines.

@t-bltg
Copy link
Collaborator Author

t-bltg commented Sep 8, 2023

With GLMakie:

glmakie

Lines appear, but missing 2 contour labels.

Missing lines on CairoMakie.

@t-bltg t-bltg marked this pull request as ready for review September 8, 2023 06:42
@t-bltg
Copy link
Collaborator Author

t-bltg commented Sep 8, 2023

I'm marking as ready for review because I have no ideas left currently.
The contour algorithm output is different whether there is a NaN or not.

@Lightup1
Copy link

Lightup1 commented Sep 8, 2023

Dear @t-bltg ,
Is there any quick fixing way to make Makie ignore NaN?
I tried to replace NaN with 1e6 in some cases it works fine, but in some other cases (when the border is smaller than the specified levels) it will draw multiple lines near the border between normal values and NaN.
Does julia support void element in a matrix which is ignored by Makie?

@t-bltg
Copy link
Collaborator Author

t-bltg commented Sep 8, 2023

Does julia support void element in a matrix which is ignored by Makie?

I would want missing (or nothing) to behave how you describe unfortunately it doesn't work either in this case.

EDIT: on hold since it breaks the examples of #2496.

EDIT2: fixed.

@t-bltg t-bltg marked this pull request as draft September 8, 2023 07:56
@jkrumbiegel
Copy link
Member

I don't think contour algorithms work with values you want to "ignore", NaNs always cause these gaps because in their surroundings the interpolation is not defined. However, if you want that behavior you can linearly interpolate the points yourself, as linear interpolation is what the contour algorithm does anyway.

@t-bltg t-bltg marked this pull request as ready for review September 10, 2023 13:06
@jkrumbiegel
Copy link
Member

Aside from the NaN-interpolation question discussed above, is this now in mergeable state? With NaN causing the appropriate gaps?

@t-bltg
Copy link
Collaborator Author

t-bltg commented Oct 25, 2023

Current state, output from the code snippet in #3210 (comment).

CairoMakie - no Nans c
cairo_no_nans

CairoMakie - Nans d
cairo_nans

GLMakie - no Nans c
gl_no_nans

GLMakie - Nans d
gl_nans

@jkrumbiegel
Copy link
Member

Ok so CairoMakie is still buggy

@t-bltg
Copy link
Collaborator Author

t-bltg commented Oct 25, 2023

Is this inconsistency between CairoMakie and GLMakie backends happening at backend level (the way every backend deals with segments with NaNs) or is this something we can solve in Makie ?

@jkrumbiegel
Copy link
Member

This must be a NaN-handling bug in CairoMakie. It may not actually draw NaNs, they have to be skipped (but one has to be careful to create the right paths in the process). And here it seems it's drawn (or something else that doesn't work) because the axis lines are missing, so Cairo broke in the process of drawing this.

@t-bltg
Copy link
Collaborator Author

t-bltg commented Oct 25, 2023

Oh right I didn't notice the axis lines missing.
Do you have any hint where this might occur ?
I'll investigate before the end of the week ...

@jkrumbiegel
Copy link
Member

Yeah probably in the code I refactored a while ago, it's a bit more complex because it tries to draw lines together as long as they have the same color and width. https://github.com/MakieOrg/Makie.jl/blob/master/CairoMakie/src/primitives.jl#L207-L288

@t-bltg
Copy link
Collaborator Author

t-bltg commented Dec 23, 2023

@jkrumbiegel, this is the new output, the bug was lying in the routine drawing glyphs.

The output is now consistent across CairoMakie and GLMakie, so I think this PR can be reviewed again (pending CI state).

GLMakie - no NaNs c
gl-no-nans-c
GLMakie - NaNs d
gl-nans-d

CairoMakie - no NaNs c
cairo-no-nans-c

CairoMakie - NaNs d
cairo-nans-d

@jkrumbiegel
Copy link
Member

Could you remove all the non functionality related formatting changes? Makes it harder to see what's up. Also the empty ifs with comments can be left as they are, I know they technically don't do anything but they helped me understand what the code was doing.

@t-bltg
Copy link
Collaborator Author

t-bltg commented Dec 23, 2023

Could you remove all the non functionality related formatting changes?

Done.

Makes it harder to see what's up.

I agree for the review, although these changes were make to simplify and make the current code consistent.

Also the empty ifs with comments can be left as they are

Done.

@t-bltg t-bltg changed the title fix contour boundingbox with NaNs fix contour boundingbox with NaNs Dec 23, 2023
@jkrumbiegel
Copy link
Member

Thank you! Looks good to me

@jkrumbiegel jkrumbiegel merged commit d39f4cb into MakieOrg:master Dec 24, 2023
15 checks passed
@t-bltg t-bltg deleted the cont_bug_nan branch December 24, 2023 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Contour errored when data contain NaN and labels=true
3 participants