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

circleProgressiveLayout() produces layouts with overlapping circles. #2

Closed
JeffreyBLewis opened this issue Jan 1, 2018 · 13 comments
Closed
Assignees
Labels

Comments

@JeffreyBLewis
Copy link

I have noticed that circleProgressiveLayout() does not ensure circles are not overlapping as the
following example demonstrates:

library(packcircles)
library(ggplot2)
areas <- c(656, 1, 41, 240, 366, 62, 259, 94, 315, 310, 140, 154, 83,
202, 211, 145, 83, 92, 284, 22, 105, 61, 17, 10, 85)
packing <- circleProgressiveLayout(areas)
dat.gg <- circleLayoutVertices(packing, npoints=50)
ggplot(data = dat.gg) + geom_polygon(aes(x, y, group = id), colour = "black",
fill = "grey90", alpha = 0.7, show.legend = FALSE) +
geom_text(data = packing, aes(x, y), label = 1:nrow(packing)) +
coord_equal() + theme_void()

Here is the result:
image

It is pretty clear what is happening, but I am not sure why. It is not a problem in @mbedward's port to R as the same layout is produced using @pmenzel's packCircles.

@mbedward
Copy link
Owner

mbedward commented Jan 1, 2018

Thank you for spotting this and for checking against @pmenzel's reference code. I haven't seen this behaviour and it hasn't been reported previously. I wonder what it is about the test data that provoke the bug? I'll look at it next week when I'm back at work.

(sorry - closed issue by mistake)

@mbedward mbedward closed this as completed Jan 1, 2018
@mbedward mbedward reopened this Jan 1, 2018
@mbedward mbedward self-assigned this Jan 1, 2018
@mbedward mbedward added the bug label Jan 1, 2018
@JeffreyBLewis
Copy link
Author

JeffreyBLewis commented Jan 1, 2018

Thank you. The example is not particular. I am writing some code to choose the (almost) most efficient packing of a set of circles inside a larger circle as part of a project to make @mbostock -like nested packed circle plots of hierarchical data. I was drawing random sets of 50 areas from an exponential distribution for testing when I found this unexpected behavior. With overlapping circles, the packing efficiency can exceed 100 percent and my not-so-clever simulated annealing optimizer can find an ordering of the areas that produces greater than 100 percent efficiency in many of the simulated exponentially distributed area data sets.

@pmenzel
Copy link

pmenzel commented Jan 2, 2018

Hi,

interesting find. It looks like the first three circles are alright and it goes down hill from the fourth.

Was it only that particular example or you found many more problems like this?

Did you check if the problem also occurs in the Circle Packing code in https://github.com/d3/d3 ?
Cause my code is based on the implementation in protovis library, which is the predecessor of D3..

@pmenzel
Copy link

pmenzel commented Jan 2, 2018

I think, I found the reason: when the 4th circle is placed, one placement puts it right within the first circle and the function that checks for circle overlap does not detect that full containment. Will thiink about a fix..

@mbostock
Copy link

mbostock commented Jan 2, 2018

There have been substantial improvements and bug fixes to the implementation of circle packing in d3-hierarchy (both 1.0 and subsequent releases). If you are basing your code off my implementation I would recommend looking at the latest code in d3-hierarchy. See release notes and related issue threads:

https://github.com/d3/d3-hierarchy/releases

@JeffreyBLewis
Copy link
Author

JeffreyBLewis commented Jan 2, 2018

In Mike Bostock's d3, the areas given above do not generate overlaps:

image

(Note that in @mbostock's d3 examples the data are presorted from largest to smallest and so when exploring the order of placement in those examples that sort must be removed. Also. note that I had to change the 2nd circle to have area=2 rather than 1 to get it displayed (not sure why), but having the second area equal 2 creates overlaps in packCircles).

@JeffreyBLewis
Copy link
Author

It appears that @mbostock addressed this problem in d3 about a year ago. The issue thread
d3/d3-hierarchy#74 works through the problem and the solution.

@mbedward
Copy link
Owner

mbedward commented Jan 3, 2018

Many thanks for the detective work Jeffrey. It looks like it should be straightforward to fix the R package following the changes for d3:

d3/d3-hierarchy@102024d

I won't get to it until next week. If you are in a hurry please submit a PR and I'll try to look at it sooner.

@JeffreyBLewis
Copy link
Author

JeffreyBLewis commented Jan 3, 2018 via email

@pmenzel
Copy link

pmenzel commented Jan 4, 2018

Hi Michael and Jeffrey,

I changed the code in packCircles and hopefully fixed the issue.

Peter

mbedward added a commit that referenced this issue Jan 9, 2018
Fixes problem where progressiveCircleLayout could return overlapping circles. Based on corresponding fix in pmenzel/packCircles@3da263d
@mbedward
Copy link
Owner

mbedward commented Jan 9, 2018

I just committed a fix based on Peter's code changes. circleProgressiveLayout now works correctly with Jeffrey's test data. If you have time, please install the fixed version (0.3.1) with devtools::install_github and let me know if there are any remaining problems. If all is well I'll submit the update to CRAN this week. Thanks everyone.

@JeffreyBLewis
Copy link
Author

JeffreyBLewis commented Jan 9, 2018 via email

@mbedward
Copy link
Owner

mbedward commented Jan 9, 2018

Great to hear - thanks very much for testing it Jeffrey. I'll close this now and submit the update to CRAN today.

@mbedward mbedward closed this as completed Jan 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants