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

New vigs review #948

Merged
merged 14 commits into from
Oct 25, 2024
Merged

New vigs review #948

merged 14 commits into from
Oct 25, 2024

Conversation

Nowosad
Copy link
Member

@Nowosad Nowosad commented Oct 25, 2024

Hi @mtennekes, I read most of the vignettes and made some changes that you can see here.

Some specific comments:

  • Vignettes 6, 8, and 10 are either outdated, missing, or contain old tmap code.
  • Vignette 2 has a statement: "there are only four arguments.", but I think that five arguments actually exist
  • Vignette 7: is it possible to have a legend that consists only of a title and a histogram? I think it may be useful in some cases...
  • Vignette 1: should this text use future or present tense (tmap v4 will vs tmap v4 is) -- I think I prefer the latter
  • Vignette 1: in a few places you use the "aesthetic" term as a synonym of "visual variable" -- maybe it would be good to stick to one of the terms?
  • Vignette 1: is tm_place_legends_right(width = 0.2) needed for the code examples? It is not explained thus I would avoid it
  • Vignette 1: all of the examples of tmap v3 in the "## Scales" section return the same legend... is this a bug?
  • Vignette 1: "An example is the bivariate choropleth, which is not yet implemented, but will definitely be." isn't it already implemented?
  • Vignette 1: the tmap4 example in the "Combined legends" section does not show any circle for the first range of values... is this a bug?

@Nowosad Nowosad requested a review from mtennekes October 25, 2024 12:25
@nickbearman
Copy link
Contributor

nickbearman commented Oct 25, 2024

Thanks @Nowosad these are looking great.

Looking at it from the point of view of a complete newbie (which is who I often teach):

tm_shape(World) + tm_polygons(fill = "HPI", fill.scale = tm_scale_intervals(values = "purple_green"), fill.legend = tm_legend(title = "Happy Planet Index"))

Renders the map with the legend underneath, rather than at the side (as below). Is there some hidden config, or Windows being weird? (I am on Windows, and installed the dev version of tmap just now).

image

This also happens with the Life_exp_class example and most of the others, but interestingly not with the Cartogram one!

Thanks for sharing these. I have pushed a minor change in wording to GitHub.

@mtennekes
Copy link
Member

Thx @Nowosad I haven't updated the vignettes yet, just restructured a bit. I was doubting between this approach or start from scratch.

Agree with present tense, and indeed we use visual variables rather than aesthetics. Thx for the commit.

@nickbearman good question! The placement of the legend depends on the aspect ratio of the device and the map. It chooses the position that maximizes the map size. In your example the map will be smaller if the legend is on the right hand side. Good that you mention it: we should emphasize this in one of the main vignettes.

@mtennekes mtennekes merged commit dad13dd into master Oct 25, 2024
7 checks passed
@Nowosad Nowosad deleted the new_vigs_jnrev branch October 25, 2024 17:34
@nickbearman
Copy link
Contributor

Thanks @mtennekes - I have added a bit of text on legends to 01_tmap_intro, in this PR #953

@nickbearman
Copy link
Contributor

I was also going to look at 06_tmap_legends, but the Rmd wouldn't compile for me.

It's the last code block:

tmap_options("values.var")$values.var$fill

which gives an error:

`processing file: 06_tmap_legends.Rmd

Error in render():
! unused arguments (visible = TRUE, envir = parent.frame())
Backtrace:

  1. rmarkdown::render(...)
  2. knitr::knit(knit_input, knit_output, envir = envir, quiet = quiet)
  3. knitr:::process_file(text, output)
  4. knitr:::process_group(group)
  5. knitr:::call_block(x)
    ...
  6. evaluate:::evaluate_call(...)
  7. knitr (local) value_fun(ev$value, ev$visible)
  8. knitr (local) fun(x, options = options)
  9. knitr:::knit_print.default(x, ...)
  10. evaluate (local) normal_print(x)

Quitting from lines 242-243 [unnamed-chunk-13] (06_tmap_legends.Rmd)
Execution halted`

If I run the code in a script, it works fine and gives this output:

`>tmap_options("values.var")$values.var$fill
$seq
[1] "-hcl.blues3"

$div
[1] "pu_gn_div"

$unord
[1] "cols4all.area7"

$ord
[1] "-hcl.blues3"

$cyc
[1] "tol.rainbow_pu_rd"

$biv
[1] "pu_gn_bivs"`

Any idea on the Rmd issue? I was going to have a bit more of a play, but ran out of time!

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.

3 participants