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

linestack with italic lables #195

Closed
eduardszoecs opened this issue Sep 1, 2016 · 14 comments
Closed

linestack with italic lables #195

eduardszoecs opened this issue Sep 1, 2016 · 14 comments

Comments

@eduardszoecs
Copy link
Contributor

Why?

For species names in linestack() it sometimes might be desirable to plot them in italics.

linestack(c(1, 2), c('Genus Species', 'Genus Species'), font = 3)

To plot all labels in italics we can use

linestack(c(1, 2), c('Genus Species', 'Genus Species'), font = 3)

Problem

But this does not work if we mix italics and non-italics:

linestack(c(1, 2), c('Genus Species', 'Genus Species'), font = c(1, 3))
# Error in plot.window(...) : 
#   graphical parameter "font" has the wrong length

A possible solution would be a expression:

linestack(c(1, 2), c('Genus Species', expression(italic('Genus Species'))))

But this parses wrongly, because labels= are converted internally to characters.

Possible Fix

A quick fix would be to remove the coercion to character or add an exception for expressions (Lines 13 -14 of linestack.R.

labels are downstream only used once in strheight() and I think in most cases the expression has the same height as the labels (?!).

May be there are also other options?

@jarioksa
Copy link
Contributor

jarioksa commented Sep 3, 2016

It seems that other expressions may fail as well. With example(linestack) the expression on graphics fail in my Mac:
linestack
git blame singles out commit fd8a2b6 by @gavinsimpson

Obviously we do not handle expressions gracefully (at least not in macOS with current R release). This should be fixed.

It seems that strheight function in R gives the space needed for the line of possible characters, because the function gives the same height for lowercase characters (strheight("x")), characters with descents (strheight("qt")) and uppercase (strheight("Å")). So we could probably get only ex <- strheight(x) -- provided that expressions also honour standard font settings, and no other argument varies the text height.

@jarioksa jarioksa added this to the 2.4-1 milestone Sep 3, 2016
@jarioksa
Copy link
Contributor

jarioksa commented Sep 3, 2016

It seems that instead of putting git blame on @gavinsimpson, you should put the blame on me: Lines 13-14 that @Edild mentions as breaking the use of mixed fonts and expressions were added by me in 6123e77 . The commit message is "Failed when names were missing and labels= was given", so this is the issue that must be considered when touching these lines.

This commit was released in vegan-2.3-4 and the example with expression() has been broken since then.

This is a regression that must be fixed.

@eduardszoecs
Copy link
Contributor Author

eduardszoecs commented Sep 3, 2016

If have currently

  if (!is.character(labels) & !is.expression(labels))
    labels <- as.character(labels)

but this might be not a good idea because strheight() gives different values with expressions (don't know why??):

strheight('s')
# 0.08686327
strheight('t')
# 0.08686327
strheight('g')
# 0.08686327
strheight('stg')
# 0.08686327

strheight(expression('s'))
#  0.06514745
strheight(expression('t'))
#  0.08686327
strheight(expression('g'))
# 0.09410188
strheight(expression('stg'))
# 0.1158177

strheight(as.character(expression('s')))
# 0.08686327
strheight(as.character(expression('t')))
# 0.08686327
strheight(as.character(expression('g')))
# 0.08686327
strheight(as.character(expression('stg')))
# 0.08686327

A possible better fix would be to coerce to character only for strheight, that is:
remove lines 13+14 and add as.character to to line 38.

@gavinsimpson
Copy link
Contributor

As for why the strheight()s differ, ?strheight has:

Note that the ‘height’ of a string is determined only by the
number of linefeeds (‘"\n"’) it contains: it is the (number of
linefeeds - 1) times the line spacing plus the height of ‘"M"’ in
the selected font.  For an expression it is the height of the
bounding box as computed by plotmath.

I would suggest we just go with whatever strheight() returns - expressions will get more space around them but will do the right thing for complex expressions with fractions, braces, and the like.

I'll take a look as the original contributor of the expression code in linestack (unless @jarioksa you've already started?).

@gavinsimpson gavinsimpson self-assigned this Sep 3, 2016
@gavinsimpson
Copy link
Contributor

Vector font argument is failing because ... is passed to plot() as well as text(). Not sure a good way to handle this; we could check if font is in ... and if so ensure it to is a length-1 vector, but that way opens up the possibility that we end up checking for all manner of parameters. (and potentially a do.call() or some other manipulation to fiddle with calling plot with a modified set of ... args.

expression(italic(foo), italic(bar)) is a good solution but verbose.

I'm inclined to not fix the font thing and suggest expression for such purposes.

Thoughts?

@gavinsimpson
Copy link
Contributor

gavinsimpson commented Sep 4, 2016

Or; it would make more sense to add font as an argument to linestack() explicitly used only in the text calls, especially as

  1. vegan is all about the species, and
  2. the !add setting is trivial to replicate if you really want a linestack plot with font length > 1

I've fixed the expression issue. I'll implement the proposal here [in branch issue-#195 *] (font as an argument) as a separate branch off branch issue-#195 so it's easy to merge if you are in agreement but otherwise won't affect the fix for expression-handling in linestack.

@gavinsimpson
Copy link
Contributor

The above are now done:

  1. fix expression handling (via 2bab7ca and 356b6d9), and
  2. added font argument to linestack (via 7e99075)

@jarioksa
Copy link
Contributor

jarioksa commented Sep 5, 2016

issue-#195 branch seems to fix the bug that I introduced. That should be merged.

You have font handling in a separate branch. I must think about that. I think it would not go to 2.4-1. One tiny problem is that this font handling would open up the door for demanding similar in all ordination text functions...

@eduardszoecs
Copy link
Contributor Author

Thanks @gavinsimpson!

I just came up with font as one solution to this problem.
As expressions now work, I don't think there is a need for font to be working inside linestack() (with all problems that come along...).

@jarioksa
Copy link
Contributor

jarioksa commented Sep 6, 2016

@gavinsimpson , would issue-#195 branch be OK for the 2.4-1 release? What ever we do with the fonts argument, that could be left for later releases.

I have now made all Fortran fixes -- including the error-prone DO loop termination stuff. I think this is the only open issue that needs solution for the 2.4-1 release.

@gavinsimpson
Copy link
Contributor

gavinsimpson commented Sep 6, 2016

@jarioksa Yes, issue-#195 is ready to merge and should go into 2.4-1. I'll merge it into master so it can get pulled into the 2.4-1 branch.

@jarioksa jarioksa removed this from the 2.4-1 milestone Sep 7, 2016
@jarioksa
Copy link
Contributor

jarioksa commented Sep 7, 2016

bug fix part was merged to cran-2.4 branch and the milestone was removed. Feature-request part is still valid.

@jarioksa jarioksa added fixed and removed bug labels Oct 21, 2016
@eduardszoecs
Copy link
Contributor Author

I think the feature-request part can be closed, as there is a practical solution?

@jarioksa
Copy link
Contributor

Yes indeed, the issue-#195 branch was merged to master long ago.

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

No branches or pull requests

3 participants