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 in ggcoef_compare() when using an include argument #448

Merged
merged 7 commits into from
Oct 7, 2022

Conversation

larmarange
Copy link
Contributor

Fix #447

@codecov-commenter
Copy link

codecov-commenter commented Aug 30, 2022

Codecov Report

Merging #448 (cd41f16) into master (53c1f65) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #448   +/-   ##
=======================================
  Coverage   92.08%   92.09%           
=======================================
  Files          36       36           
  Lines        4361     4362    +1     
=======================================
+ Hits         4016     4017    +1     
  Misses        345      345           
Impacted Files Coverage Δ
R/stat_weighted_mean.R 87.71% <ø> (ø)
R/ggcoef_model.R 98.26% <100.00%> (+<0.01%) ⬆️
R/ggnetworkmap.R 85.89% <100.00%> (ø)
R/ggparcoord.R 96.96% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@larmarange
Copy link
Contributor Author

Note: I have also updated documentation of stat_weighted_mean() and the way to test class inheritance in two files, in order to pass the checks.

@larmarange
Copy link
Contributor Author

Dear @schloerke

This PR has been ready for review since the end of August. The last version of GGally was released in June 2021. Therefore, the last bug fixes from January 2022 are not yet available for users.

I know that package maintenance could be a challenge, but it could be relevant to submit a new version of GGally.

Regarding the maintenance of the ggcoef_*() family of functions, would it be easier for you if I develop and submit on CRAN a standalone package with just these functions, and then if we import them in GGally? (I know that the drawback would be to increase the number of dependencies for GGally but on the other side we could remove some dependencies needed only for the ggcoef_*() family).

To be noted, it would also require to move geom_stripped_rows() and geom_stripped_cols() to this new package and to reimport them in GGally.

Please let me know what you prefer

@schloerke
Copy link
Member

but it could be relevant to submit a new version of GGally.

@larmarange Yes! Especially with #451 . I'll start prep today.


Should we make a {ggcoef} package and reexport within {GGally}?

Sorry that I've been slow on releases. Yes, you do a great job fixing bugs very quickly. I'd be happy to reexport any methods from this R package.

Example syntax

#' @export
PACKAGE::METHOD

If you'd like for me to wait on releasing {GGally} so that your new package re-exports can be made, let me know. 😃

@schloerke schloerke merged commit e4217fe into ggobi:master Oct 7, 2022
@larmarange larmarange deleted the issue447 branch October 7, 2022 13:25
schloerke added a commit to huizezhang-sherry/ggally that referenced this pull request Oct 12, 2022
* master:
  Use `{vdiffr}`; Update GHA; Update "print on CI / examples" method; Fix lints (ggobi#453)
  Fix in `ggcoef_compare()` when using an `include` argument (ggobi#448)
  Fix no_reference_row option in ggcoeff_compare() (ggobi#431)
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.

ggcoef_compare - include= doesn't work: idx must contain one integer for each level of f
3 participants