-
-
Notifications
You must be signed in to change notification settings - Fork 15
Adds back support where 2 categorical variables are used in association and bivariate plots #949
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
Conversation
Unit Tests Summary 1 files 24 suites 15m 59s ⏱️ Results for commit 5db4df3. ♻️ This comment has been updated with latest results. |
Unit Test Performance Difference
Additional test case details
Results for commit 694b007 ♻️ This comment has been updated with latest results. |
llrs-roche
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great PR that we bring back the plot without extra dependencies or too much code! Thanks!!
However, please generated the required data for the plot outside the plot call itself. It will help with maintainability and readability. Related to readability please use %>% when you move the data preparation outside the plot generation.
On the future we should consider using {patchwork} and combining the plots with it is much more ggplot2 friendly than using gridExtra: we only need to use plot1/plot2 to get the final ggplot2 that will print without problems. This is not a requirement as I don't want to add a new extra dependency but just for us to be aware.
Code Coverage SummaryDiff against mainResults for commit: 5db4df3 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
* origin/main: [skip actions] Bump version to 0.5.1.9015 increase timeouts (#950)
# Pull Request WIP: using "ready" status for testing purposes Fixes #nnn - Detected these errors when testing #944 ### Changes description: - [x] `tm_viewer` throws warning of URL - [x] ~`tm_g_association` example shows error in application~ fixed in #949 - [x] `tm_variable_browser` example cannot find `%>%` --------- Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
llrs-roche
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just commenting the code as I haven't checked it interactively as the CI is still failing for the PR.
Is the new geom compatible with older ggplot2 versions?
I think we are almost done with this.
Co-authored-by: Lluís Revilla <[email protected]> Signed-off-by: André Veríssimo <[email protected]>
llrs-roche
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e2e test are failing due to issues on tm_file_viewer and tm_missing_data if #922 is merged first this would be fixed. If not, you can inspire from how the tests were modified to be a bit more flexible but still test what we want
I see you use .data inside dplyr calls. I'd like to be consistent and not use them unless needed.
The visualizations look as good as they were before (I hope TMG won't get dependencies only for geom_mosaic() :D )
Pull Request
Fixes #948
Changes description
ggplot2layers are created%>%instead for have more readability