Makie.jl icon indicating copy to clipboard operation
Makie.jl copied to clipboard

fix `hexbin` corner case - reorganize test files

Open t-bltg opened this issue 1 year ago • 6 comments

Description

Fix https://github.com/MakieOrg/Makie.jl/issues/3357. Fix https://github.com/MakieOrg/Makie.jl/issues/3507.

Simplified a bit the implementation of hexbin (mostly cleanup, unused leftovers, ...).

Type of change

Delete options that do not apply:

  • [x] 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
  • [x] Added unit tests for new algorithms, conversion methods, etc.
  • [ ] Added reference image tests for new plotting functions, recipes, visual options, etc.

t-bltg avatar Dec 24 '23 10:12 t-bltg

This is the output from the added tests.

Not sure if we want to fix the case with all ones (empty plot because the markersize is too small, w.r.t. x or y limits).

# degenerate case with singleton 0
save("x0.png", hexbin([0, 0], [1, 2]))
save("y0.png", hexbin([1, 2], [0, 0]))

# degenerate case with singleton 1
save("x1.png", hexbin([1, 1], [1, 2]))
save("y1.png", hexbin([1, 2], [1, 1]))

x0 x0 y0 y0

x1 x1 y1 y1

t-bltg avatar Dec 24 '23 10:12 t-bltg

For https://github.com/MakieOrg/Makie.jl/issues/3507, I've sorted the list of tests to be ran in alphabetical order: this is less error prone.

Fixing statistical_tests.jl and zoom_pan.jl is left for another PR, but at least they are now listed in runtests.jl with a FIXME note added.

t-bltg avatar Dec 24 '23 11:12 t-bltg

I fail to understand why the Relocability CI test fails all the time now :/

EDIT: switching from ubuntu-20.04 to ubuntu-22.04 in CI job seems to fix it :shrug:.

t-bltg avatar Dec 24 '23 12:12 t-bltg

I fail to understand why the Relocability CI test fails all the time now :/

Hum, spurious random CI failure, restored the old ubuntu-20.4.

t-bltg avatar Dec 28 '23 13:12 t-bltg

Hm I find it weird that whether the singleton dimension has only zeros or ones makes a difference whether bins are shown or not. I'm really not sure what a good behavior here is. Also, in the first case, it's kind of weird that the upper bin would include the point in question right on the edge basically. Because the visual impression is that there's some data to the right.

grafik

Actually I've noticed that the way the bins are chosen by default doesn't quite make sense in the y direction:

grafik

The bins extend further than they need to, vertically, to fully envelop the rectangle with the data points. Maybe we should also fix that?

We should check what matplotlib and ggplot are doing with their hexbins for these cases.

jkrumbiegel avatar Dec 28 '23 19:12 jkrumbiegel

These are examples from matplotlib, adapted from the hexbin docs:

import numpy as np
import matplotlib.pyplot as plt

np.random.seed(19680801)
n = 300
x = np.random.rand(n)
y = np.random.rand(n)

fig, ax = plt.subplots()
ax.hexbin(x, y, gridsize=(3, 4 // 2))
fig.savefig('py-3x4.png')

fig, ax = plt.subplots()
ax.hexbin([0, 0], [1, 2], gridsize=(2, 2 // 2))
fig.savefig('py-x0.png')

fig, ax = plt.subplots()
ax.hexbin([1, 2], [0, 0], gridsize=(2, 2 // 2))
fig.savefig('py-y0.png')

fig, ax = plt.subplots()
ax.hexbin([1, 1], [1, 2], gridsize=(2, 2 // 2))
fig.savefig('py-x1.png')

fig, ax = plt.subplots()
ax.hexbin([1, 2], [1, 1], gridsize=(2, 2 // 2))
fig.savefig('py-y1.png')

py-3x4 py-3x4 py-x0 py-x0 py-y0 py-y0 py-x1 py-x1 py-y1 py-y1

Here, it seems a value of 0.1 is added to both sides of the singleton by default.

The bins extend further than they need to, vertically, to fully envelop the rectangle with the data points. Maybe we should also fix that?

I don't really understand what you mean: this seems to be consistent with what matplotlib does, no ?

Hm I find it weird that whether the singleton dimension has only zeros or ones makes a difference whether bins are shown or not.

Agreed.

I think we should target uniform hex sizes for singleton cases.

t-bltg avatar Jan 07 '24 15:01 t-bltg