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 contourf Inf value on non-extended case #3684

Closed
wants to merge 5 commits into from

Conversation

t-bltg
Copy link
Collaborator

@t-bltg t-bltg commented Mar 6, 2024

Description

Fix #3683.

x = y = LinRange(0, 1, 10)
ymin, ymax = 0.4, 0.6
steepness = 0.1
f(x, y) = (tanh((y - ymin) / steepness) - tanh((y - ymax) / steepness) - 1)
z = [f(_x, _y) for _x in x, _y in y]

fig, ax, cof = contourf(x, y, z)
Colorbar(fig[1, 2], cof)
save("noext.png", fig)

fig, ax, cof = contourf(x, y, z; extendhigh=:auto)
Colorbar(fig[1, 2], cof)
save("ext.png", fig)

noext
noext

ext
ext

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 CHANGELOG.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 added the bug label Mar 6, 2024
@t-bltg t-bltg force-pushed the contourf-inf branch 2 times, most recently from e55b178 to f64fbc1 Compare March 6, 2024 17:24
src/basic_recipes/contourf.jl Outdated Show resolved Hide resolved
src/basic_recipes/contourf.jl Outdated Show resolved Hide resolved
@t-bltg t-bltg requested a review from jkrumbiegel March 6, 2024 19:06
@jkrumbiegel
Copy link
Collaborator

I don't think this fixes the real bug, in my understanding contourf(x, y, z) should also have the gap filled, no? For some reason, one of the isobands is treated like it's out of range.

When I do highs[end] += eps(highs[end]) after the line highs = levels[2:end] it actually fills the gap.

@t-bltg
Copy link
Collaborator Author

t-bltg commented Mar 6, 2024

Isn't the root of this bug due to the Float32 -> Float64 conversion in https://github.com/jkrumbiegel/Isoband.jl/blob/28058449fb3c08dc531d851686e920fe6ca3138e/src/Isoband.jl#L27 ?

Should https://github.com/jkrumbiegel/isoband/blob/6994b367c7af3c0e31c8c4b103850772bff818c6/src/isoband.cpp#L1701C26-L1701C39 also be implemented for floats since Makie is mostly Float32 based ?

@t-bltg t-bltg changed the title fix Inf value on non-extended case fix contour Inf value on non-extended case Mar 9, 2024
@t-bltg t-bltg changed the title fix contour Inf value on non-extended case fix contourf Inf value on non-extended case Mar 9, 2024
@jkrumbiegel
Copy link
Collaborator

I don't think Float32 to Float64 conversion is the problem. Any Float32 is directly representable in Float64 I think, so there should be no mismatch. Somethings definitely wrong here though, would be good to get that logic straight

@t-bltg
Copy link
Collaborator Author

t-bltg commented Mar 13, 2024

Let's try jkrumbiegel/Isoband.jl#2.

Using float doesn't hurt (faster).

@jkrumbiegel
Copy link
Collaborator

superseded by #3713

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.

contourf bug
2 participants