-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Review {Hmisc} dependency #5230
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
Comments
Sorry for noticing this trouble late. If the intent is just to pass the CI, I think we don't need to desperate to drop Hmisc. In my understanding, the tidyverse's "4 previous versions" rule doesn't mean it maintains all the functionalities including the ones that need the Suggest dependencies. In fact, ggplot2 already gave up some packages on the older versions. ggplot2/.github/workflows/R-CMD-check.yaml Lines 66 to 67 in d6d0523
But, considering we have repeatedly faced such issues caused by the different support policies, it might be a good idea to implement these functions within ggplot2. |
It would be nice if we could pass the CI, but I think that is a secondary issue. I think the future-proofing and ecosystem bits are of some importance though. This probably isn't limited to just {Hmisc}. There are some other suggested packages that I cannot find any (or very limited) code references to (some are mentioned in docs though). Examples of these are {munsell}, {RColorBrewer}, {lattice}, {rpart}, {rgeos}. And there are suggested packages who are used in code, but whose scope is limited. For example {xml2} is just used in one unit test, {ggplot2movies} or {nlme} is only used in examples, and {multcomp} only has some fortify methods. Maybe this is all a roundabout way to say that I'm not exactly clear what the suggest dependency policy is 😅 |
Yeah, I too am not sure about the policy...
Good catch. It seems lattice is what I forgot to remove on #4079 (it seems lattice was used only for explaining how to translate lattice plots to the ggplot2 code). |
I'm going to consider this fixed by #5237. |
This issue is mostly prompted by recent CI failures due to the Hmisc package switching to use a few native pipes (more details at harrelfe/Hmisc#159, a PR for a fix might be underway). Since tidyverse packages are expected to support non-native-pipe versions of R (< 4.1.0) up to 2025, it might be best to explore whether ggplot2 can drop Hmisc from the 'Suggests' list.
In addition to the reason above, this paper https://arxiv.org/abs/2208.11674 describes {Hmisc} as a 'heavy parent' package (and off-topic: ggplot2 as a 'hub' package). It might be good for R package ecosystem health if ggplot2 did not depend on {Hmisc} if it is a heavy parent. Moreover, it seems also wise to prevent a similar issue as with the {isoband} package down the line.
As far as I can tell, the {Hmisc} package is needed for the
mean_cl_boot()
,mean_cl_normal()
,mean_sdl()
andmedian_hilow()
functions over here:ggplot2/R/stat-summary.R
Lines 248 to 273 in 04a5ef2
It might not be terribly inconvenient to offer a few homegrown alternatives for these 4 functions. Any thoughts?
The text was updated successfully, but these errors were encountered: