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

Fix plot labels with spaces #298

Merged
merged 5 commits into from Jan 20, 2022
Merged

Fix plot labels with spaces #298

merged 5 commits into from Jan 20, 2022

Conversation

Enchufa2
Copy link
Member

@Enchufa2 Enchufa2 commented Jan 20, 2022

Closes #297. @edzer I simplified a bit the logic to add units to labels, with some changes:

  • I don't think we should export make_unit_label anymore. This was probably exported to enable ggforce to set the labels easily, right? But it was not used correctly anyway. Now that we have included ggforce's functionality, I have renamed the function as make_unit_label_internal and deprecated direct calls to make_unit_label (which calls the former one) if this is ok with you. Or are you aware of any other package really requiring make_unit_label? See below.
  • The specific fix for Development version of units seems to fail with spaces in labels #297 is included in make_unit_label, which replaces spaces with ~ if the label is a string.
  • Previously, units were not added if the user provided a custom label (via xlab, ylab). Instead, I think that we should always add units, as implemented in this PR. And if the user do not want units, they should just drop them.

@Enchufa2
Copy link
Member Author

Examples:

library(units)
#> udunits database from /usr/share/udunits/udunits2.xml
library(ggplot2)

mtcars$consumption <- set_units(mtcars$mpg, mi / gallon)
mtcars$power <- set_units(mtcars$hp, hp)

plot(consumption~power, mtcars, xlab="asdf asdf")

p <- ggplot(mtcars) + geom_point(aes(power^2, consumption))
p + xlab("some^2 custom stuff")

p + xlab(NULL)

Created on 2022-01-20 by the reprex package (v2.0.1)

@codecov
Copy link

codecov bot commented Jan 20, 2022

Codecov Report

Merging #298 (66590e7) into main (729bc9c) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #298   +/-   ##
=======================================
  Coverage   92.24%   92.25%           
=======================================
  Files          19       19           
  Lines         916      917    +1     
=======================================
+ Hits          845      846    +1     
  Misses         71       71           
Impacted Files Coverage Δ
R/plot.R 97.22% <100.00%> (-0.08%) ⬇️
R/scale_units.R 89.13% <100.00%> (+0.49%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 729bc9c...66590e7. Read the comment docs.

@bart1
Copy link
Contributor

bart1 commented Jan 20, 2022

Thank you for your effort @Enchufa2 I have played around a bit with specifying the labels, spaces and expressions work as expected. The results for special characters in the string (e.g. \n and \t) still differ. These are maybe not as critical to get right but if it is easy it might prevent errors for people.

require(units)
#> Loading required package: units
#> udunits database from /usr/share/xml/udunits/udunits2.xml
d<-data.frame(b=1:5,a=set_units(5:1,'Hz'))
require(ggplot2)
#> Loading required package: ggplot2
ggplot(d)+geom_point(aes(a,b))+xlab("a b")+ylab("a b")

ggplot(d)+geom_point(aes(a,b))+xlab("a\nb")+ylab("a\nb")

ggplot(d)+geom_point(aes(a,b))+xlab("a\tb")+ylab("a\tb")

ggplot(d)+geom_point(aes(a,b))+xlab(expression(frac(a,b)))+ylab(expression(frac(a,b)))

Created on 2022-01-20 by the reprex package (v2.0.1)

@Enchufa2
Copy link
Member Author

Thanks for testing. To be able to properly show powers, unit labels must be expressions, and thus we lose the ability to insert a newline or a tab character.

@bart1
Copy link
Contributor

bart1 commented Jan 20, 2022

Thanks those new lines could be solved by expressions anyway

@Enchufa2
Copy link
Member Author

There are projects using make_unit_label after all... Let's undeprecate it then. :(

@Enchufa2
Copy link
Member Author

Done, so if no further comments from @edzer, we are good to go. :)

@edzer
Copy link
Member

edzer commented Jan 20, 2022

Looks good to me - great work!

@Enchufa2 Enchufa2 merged commit f388fba into main Jan 20, 2022
@Enchufa2 Enchufa2 deleted the fix/297 branch January 20, 2022 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Development version of units seems to fail with spaces in labels
3 participants