Ensure circle size stays more consistent when zooming.#7915
Ensure circle size stays more consistent when zooming.#7915thomasneirynck wants to merge 5 commits intoelastic:masterfrom
Conversation
|
Should the circles be overlapping? |
|
@jbudz yes. I chose this to contrast with the shaded_circle markers visualization-type, where they will never overlap. They overlap because geohash cells are rectangular. I chose the longest side (width/height) for each precision. To not overlap, we choose the shortest side. Then, the visualization will be somewhat similar to shaded_circles (apart from the relative sizing). (fyi, I have no real preference for one of the other) |
There was a problem hiding this comment.
does this scale up? thoughts on bumping it to 21 or so?
There was a problem hiding this comment.
Kept it 18 because that was the limit of the previous one. Indeed, moving to 21 will be more robust.
|
As far as the overlapping here, I liked the idea of it, but I'm not crazy about it seeing it with data. For comparison: Original
Max value (overlapping circles)
Min value (non-overlapping circles)
All of these are showing the exact same data, and the later is using I might be more OK with the overlapping if the higher values were always on top of the lower ones. As it is, it's easier to see the density but really hard to see the intensity. That's more in line with the intention of the circles in this style, but still seems less useful to me... The different is subtle, but it exists: |
|
I know one of the things this PR is aiming to fix is the size of the circles when dealing with negative values. It would be nice to see at least 1 test that dealt with a mix of negative and positive values. |
|
As for removing the "overlapping", I agree, it works better. I'll change that. As for the added tests: OK, will do. |
|
For improved clarity, I will split this tick up into two, since both changes here do not have to happen together.
|
When a map was zoomed in, circle sizes would shrink quickly sometimes. This was an odd visual effect. This was due to the mapping of the the map's zoom level to the geohash's precision level growing much too quickly. The selected geohash precision level would be much to high for the corresponding scale. Also removed many of the magic numbers from the scale circle visualization. Now, circles are scaled according to their area.
The previous test verified some particular behavior of the previous implementation - each 0 value must map to a 0 radius. It is possible to have meaningful 0 values (e.g. aggregation), that should be displayed on the map. - negative values are coerced to positive. This is not entirely clear why this is the case, as it introduces some problems with ranges from negative to positive value. Also removed randomness from the test wrt zoom levels
By favoring the largest dimension of the geohash grid cell (width or height), we introduce a small overlap effect, which is visually more pleasing. Now also takes into account that geohash grid alternately divides by 4 and 8 at each precision.
1e5caa1 to
a7c0995
Compare
|
@thomasneirynck The tests are legit failing. I suspect based on that output that you didn't update the values for the use of |
|
@w33ble thanks for looking into the test! you're right, that failure is legit. @jbudz @w33ble I am closing this PR (unmerged). I feel both items addressed here are too different to keep in a single PR. Rewriting the radius-logic does not address the circle-size-issue. I created a separate ticket to track this (#8001). I also created a new, cleaner PR just for the circle-size-issue (#8000). |
Instead, the mapping of zoom level to geohash precision is now more clearly defined: it ensures that the geohash precision does not get smaller than an arbitrary number of pixels on the map. This should make the switch to different geohash precision levels feel a little more predictable.
Note that these jumps in circle-size (from large to small) cannot entirely be avoided due to the stateless nature of the visualizations. If we want sizes to stay consistent across zooming, we would need to pass some contextual information between zoom-movements so that we could map aggregations from different geohash-precisions. This is I believe not necessary.
I proposed a simplification to scale on the area of the circle, which is a fairly conventional way of sizing circle markers. The maximum diameter of a circle is now also strictly defined as the width or height of the geohash gridcell, and less based on ill-defined magic numbers.
The unit-tests explicitly tested the assumptions above, so I modified those as well.