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

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

gvegayon
Copy link

@gvegayon gvegayon commented Oct 6, 2016

Following issue #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
Copy link
Author

Following CONTRIBUTING.md, ping @gaborcsardi @ntamas

@vtraag
Copy link
Member

vtraag commented Mar 5, 2021

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 😉

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.

@gvegayon
Copy link
Author

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.

@szhorvat
Copy link
Member

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.

@krlmlr krlmlr added the plotting 💹 Issues related to plotting label Feb 20, 2024
Copy link
Contributor

aviator-app bot commented Feb 20, 2024

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.

@krlmlr
Copy link
Contributor

krlmlr commented Mar 2, 2024

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

@gvegayon
Copy link
Author

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

@krlmlr
Copy link
Contributor

krlmlr commented Apr 9, 2024

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?

@maelle
Copy link
Contributor

maelle commented May 7, 2024

What about the comments in #172 (comment)? Do we want to make this a breaking change?

@maelle
Copy link
Contributor

maelle commented May 7, 2024

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?

@krlmlr
Copy link
Contributor

krlmlr commented Jun 20, 2024

Want to sync main after #1413 and see what remains.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plotting 💹 Issues related to plotting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants