Make bqplot linear scatter sizes more closely match matplotlib viewer

[Carifio24]

This PR aims to resolve #392 by making the linear size scaling behave more similarly to that of the matplotlib viewer. To do this, I ended up not using the size scales for the layer, but rather following the matplotlib viewer's example and capping the size.

FWIW, my initial thought was to do something like

s = ensure_numerical(self.layer[self.state.size_att].ravel())
s = (s - self.state.size_vmin) / (self.state.size_vmax - self.state.size_vmin)
np.clip(s, self.state.size_vmin, self.state.size_vmax, out=s)
s *= 0.95
s += 0.05 * (self.state.size_vmax - self.state.size_vmin)
s *= self.scatter_mark.default_size
self.scatter_mark.size = s ** 2

self.scale_size_scatter.min = float_or_none(self.state.size_vmin ** 2)
self.scale_size_scatter.max = float_or_none(self.state.size_vmax ** 2)

but that gave results that differed from the matplotlib implementation when size_vmin was nonzero (like e.g. the example data in #392).

[dhomeier]

I suppose the image comparison failure is not unexpected, as the produced figure should look different now – have to update the reference image then. Do we only need to update

glue-jupyter/glue_jupyter/tests/images/py311-test-visual.json

Lines 2 to 3 in 83aeb3a

	"glue_jupyter.bqplot.scatter.tests.test_visual.test_visual_scatter2d[chromium]": "3fe576be80889cc20063dd9e17f39899b2e40fb9a779eaa02e19b01181b332aa",
	"glue_jupyter.bqplot.scatter.tests.test_visual.test_visual_scatter2d_density[chromium]": "f09681c1d9aea95612e4a268ab9ada15f5cb7c0158bfc5b13ccb0c890779feb2"

to

"edd4e65c87369bc6e403f45e87d914223bc13f42c2f90a55535614923e233c00"
"60f38b822f89603874cd508e0c320db92185129631dd89a58624dd667eb8a5a7"

then?

[dhomeier]

Those numbers (size_scaling * 7, s ranging from 0.05 to 1.0) are taken from comparison with the matplotlib defaults?
I was wondering if the division by 0 in

scatter_density_mark.py:210: RuntimeWarning: invalid value encountered in divide
normalized_counts = (self._counts - vmin) / (vmax - vmin)

was related, but that warning already pops up prior to this PR – I suppose that's bad user input really.

[Carifio24]

Yeah, the 7 is a magic constant that I thought gave the closest match to the matplotlib layer artist - basically it's meant to serve a role similar what 30 does here.

[dhomeier]

If the parent LayerArtist is crafted that way, matching that makes sense. I'm good with the implantation so far, let's wait a little if @astrofrog has any comments.

[Carifio24]

I'm not super familiar with the CircleCI system yet, but I think you're right that we just need to update the hashes. Once we're satisfied with all the code changes I'll take care of that.

[dhomeier]

Go ahead with updating the hashes if you wish; once the CI is fixed this should be good to go in for the 0.18.0 release.

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.22% 🎉

Comparison is base (1e70916) 86.61% compared to head (807daee) 86.84%.
Report is 32 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #394      +/-   ##
==========================================
+ Coverage   86.61%   86.84%   +0.22%     
==========================================
  Files          89       89              
  Lines        4946     5077     +131     
==========================================
+ Hits         4284     4409     +125     
- Misses        662      668       +6     

Files Changed	Coverage Δ
glue_jupyter/bqplot/scatter/layer_artist.py	91.00% <100.00%> (+0.14%)	⬆️

... and 6 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[dhomeier]

I'm not super familiar with the CircleCI system yet, but I think you're right that we just need to update the hashes. Once we're satisfied with all the code changes I'll take care of that.

Thanks! Struggling to make sense of it myself – image comparison is now passing with the expected hash, though the figure_report still shows a different (the original) baseline image. From the pytest invocation I take that the reference figures are pulled from https://github.com/glue-viz/glue-jupyter-visual-tests; this in turn seems to be automatically updated when a change in py311-test-visual.json is pushed to main, so I'd just merge and see if this works.

I was more worried about the docs build still failing, but seeing now that readthedocs tries to use an obsolete .readthedocs.yml with Python 3.6. Should have rebased to pull in #386, but I think this will also sort itself out on merging.