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

draft ggplot based sankey plot #367

Merged
merged 12 commits into from
Jan 8, 2025
Merged

draft ggplot based sankey plot #367

merged 12 commits into from
Jan 8, 2025

Conversation

jacobvjk
Copy link
Member

@jacobvjk jacobvjk commented Dec 17, 2024

closes #364

  • updates implementation of sankey chart to depend on ggalluvial and ggrepel instead of webshot and networkD3
  • removal of webshot dependency should make CRAN incoming checks pass more easily
  • updates documentation (data dictionary and cook book) of the sankey chart to reflect minor changes in the output data

example of new sankey plot:
plot_sankey_sector

Copy link

github-actions bot commented Dec 17, 2024

PR Preview Action v1.4.8
Preview removed because the pull request was closed.
2025-01-08 08:26 UTC

Copy link

codecov bot commented Dec 17, 2024

Codecov Report

Attention: Patch coverage is 97.33333% with 2 lines in your changes missing coverage. Please review.

Project coverage is 79.97%. Comparing base (d1b3a22) to head (779ca3d).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
R/plot_aggregate_loanbooks.R 93.75% 1 Missing ⚠️
R/plot_sankey.R 96.29% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #367      +/-   ##
==========================================
- Coverage   80.05%   79.97%   -0.09%     
==========================================
  Files          29       29              
  Lines        3269     3186      -83     
==========================================
- Hits         2617     2548      -69     
+ Misses        652      638      -14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jacobvjk jacobvjk marked this pull request as ready for review January 6, 2025 10:56
@jacobvjk jacobvjk requested review from cjyetman and jdhoffa January 6, 2025 11:03
@jacobvjk
Copy link
Member Author

jacobvjk commented Jan 6, 2025

@cjyetman @jdhoffa the PR is unfortunately rather large as it replaces long ish sankey functions.

This is roughly what you should expect:

  • Running the analysis works exactly the same way it used to
  • The sankey plot png looks more ggplot2-ish (see image in the description of this PR)
  • the data set corresponding to the sankey plot has gained an additional column that contains the currency, as I added title and axis descriptions to the plot which required additional information
  • the rest: see above

middle_node2 = "sector"
ggplot2::ggsave(
plot = p_sankey,
filename = glue::glue("plot_{output_file_sankey_sector}.png"),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where does output_file_sankey_sector come from in this updated script?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lines 159-163

alignment_metric < 0 ~ "Not aligned",
TRUE ~ "Unknown"
),
middle_node = tools::toTitleCase(!!rlang::sym(middle_node))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure, but I would've guessed that using tools::toTitleCase() would require adding tools to the DESCRIPTION file to avoid a R CMD check warning? (I know tools is included in base R, but I thought R CMD check was picky enough about that 🤷🏻)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apparently not though, no notes in the CI checks

@jacobvjk jacobvjk merged commit c3689e1 into main Jan 8, 2025
12 checks passed
@jacobvjk jacobvjk deleted the update-sankey branch January 8, 2025 08:21
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.

upkeep: do not depend on networkD3 and webshot
2 participants