rigraph icon indicating copy to clipboard operation
rigraph copied to clipboard

Relative vertex.size + fix in tests + spelling in plot.common.Rd

Open gvegayon opened this issue 7 years ago • 6 comments

Following issue https://github.com/igraph/rigraph/issues/161, this PR makes a significan change in the way vertex.size is processed. In particular, instead of using the scaling (1/200) as seen throughout the code, plot.igraph now uses the function i.rescale.vertex which rescales the vertex size such that the smallest vertex is of size vertex.relative.size[1] and the largest one of size vertex.relative.size[2] (vertex.relative.size is a new parameter that has been added as relative.size in the list i.vertex.default and has a default values of c(.01,.025) [1%,2.5% of the plotting region]).

This new feature has been included to be compatible with rglplot and tkplot so all three plotting functions have somewhat same vertex sizes.

A neat example of this new feature is:

igraph_options(plot.layout=layout_nicely)
set.seed(312)
g <- barabasi.game(10)

oldpar <- par(no.readonly=TRUE)
par(mfrow=c(2,2))
set.seed(1);plot(g, main="Default")
set.seed(1);plot(g, vertex.size=degree(g), main="size=degree")
set.seed(1);plot(g, vertex.size=degree(g), vertex.relative.size=c(.05,.1),
                main="size=degree,relative.size=c(.05,.1)")
set.seed(1);plot(g, vertex.size=degree(g), vertex.relative.size=c(.025,.2),
                main="size=degree,relative.size=c(.025,.2)")
par(oldpar)

example

which has been included at the end of the file plot.common.Rd. Furthermore, the changes do not affect significantly examples in the plot.common.Rd file since by default all vertices occupy 2.5% of the screen.

One important drawback of this PR is that "simple" changes in vertex.size won't be reflected, e.g.

set.seed(1211)
g <- barabasi.game(10)

# These three are equivalent
oldpar <- par(no.readonly=TRUE)
par(mfrow=c(2,3), mar=rep(0,4))
set.seed(1);plot(g)
set.seed(1);plot(g, vertex.size=1)
set.seed(1);plot(g, vertex.size=2)

# But this not
set.seed(1);plot(g, vertex.size=1, vertex.shape="rectangle", vertex.size2=1)
set.seed(1);plot(g, vertex.size=1, vertex.shape="rectangle", vertex.size2=2)
set.seed(1);plot(g, vertex.size=2, vertex.shape="rectangle", vertex.size2=1)
par(oldpar)

example2

Finally, this PR fixes a couple of 'failures' that testthat showed due to a relatively recent update in which using print started to be a requirement within 'expectations'.

I understand that this may be a big change but the benefits are greater (in my opinion) since picking vertex sizes will now be more intuitive than before.

gvegayon avatar Oct 06 '16 22:10 gvegayon

Following CONTRIBUTING.md, ping @gaborcsardi @ntamas

gvegayon avatar Oct 13 '16 16:10 gvegayon

In general, this possibility looks quite useful @gvegayon, thanks for contributing! It has taken slightly longer than a week to get back to you on this :wink:

I will have to look at this in a bit more detail, but I had a more general question. The ability to specify relative vertex sizes is definitely useful, however, it is a pity that the ordinary vertex.size argument no longer works as originally. This will break some existing code of users that simply wanted to create larger (or smaller) vertices. I was wondering whether it would not be possible to do the following:

  • Rename vertex.size2 that you have now to vertex.size.
  • Rename the vertex.size to vertex.relative.size to clarify you are specifying a relative size.
  • Rename vertex.relative.size to vertex.relative.scaling to indicate the scaling of the relative vertex sizes that you have.

That way, users continuing to use vertex.size would see no difference, but users would be able to specify a (possibly more useful) vertex.relative.size, where the scaling of the vertex.relative.size is controlled by vertex.relative.scaling. Let me know what you think.

vtraag avatar Mar 05 '21 15:03 vtraag

No worries wrt to the timing, @vtraag! I see myself reflected on this 😛. Sure, that makes sense. I will take a look at this and try to implement one of your suggested solutions.

gvegayon avatar Mar 24 '21 04:03 gvegayon

If a breaking change is made, this would be an opportunity to think carefully about what the best way is to set vertex sizes. I'll admit I haven't yet looked into how this is done in the R interface yet, but for lack of time, I'll just throw a few Mathematica-inspired ideas here:

  • It is useful to set vertex size as a fraction of the shortest edge length, or as a fraction of the distance of the closes two vertices (in case R already has facilities to compute this in better than O(n^2) time).
  • It is also useful to set it as a fraction of a bounding region of all vertices (bounding box, bounding disk, whatever)
  • Finally, if R plotting generally supports this, it is also useful to set it in device units (e.g. millimeters/points for printing, pixels for screen, etc.) that is independent of plot coordinates. This is what is normally useful for text: the text size should be readable, regardless of whether the plot range is from 0 to 1 or 0 to a million.

szhorvat avatar Mar 24 '21 11:03 szhorvat

Current Aviator status

Aviator will automatically update this comment as the status of the PR changes. Comment /aviator refresh to force Aviator to re-examine your PR (or learn about other /aviator commands).

This pull request is currently open (not queued).

How to merge

To merge this PR, comment /aviator merge or add the mergequeue label.


See the real-time status of this PR on the Aviator webapp.
Use the Aviator Chrome Extension to see the status of your PR within GitHub.

aviator-app[bot] avatar Feb 20 '24 14:02 aviator-app[bot]

This now has conflicts, my apologies. Are you still interested in the change?

krlmlr avatar Mar 02 '24 15:03 krlmlr

Just solved the conflicts. I haven't tested the code, but it should still work. Will try it out tomorrow.

gvegayon avatar Mar 27 '24 03:03 gvegayon

The vignettes now fail: https://github.com/igraph/rigraph/actions/runs/8446422057/job/23140221959?pr=172#step:7:34 . Can you please take a look?

krlmlr avatar Apr 09 '24 13:04 krlmlr

What about the comments in https://github.com/igraph/rigraph/pull/172#issuecomment-791498917? Do we want to make this a breaking change?

maelle avatar May 07 '24 10:05 maelle

library("igraph")
#> 
#> Attaching package: 'igraph'
#> The following objects are masked from 'package:stats':
#> 
#>     decompose, spectrum
#> The following object is masked from 'package:base':
#> 
#>     union
g <- make_graph("Zachary")
plot(g)
#> Error in symbols(x = coords[, 1], y = coords[, 2], bg = vertex.color, : invalid symbol parameter

Created on 2024-05-07 with reprex v2.1.0

For some reason in this reprex vertex.size becomes a vector of NA, that is passed to symbols(). Any idea what could be wrong?

maelle avatar May 07 '24 11:05 maelle