jpquast / ggplate

Create Layout Plots of Biological Culture Plates and Microplates
https://jpquast.github.io/ggplate/
Other
90 stars 6 forks source link

cleaning up namespaces #2

Closed BradyAJohnston closed 1 year ago

BradyAJohnston commented 1 year ago

This cleans up some of the namespace usage.

Changes

Suggestions

I'd also suggest potentially creating a custom stat / geom for plates rather than just providing a function that plots them, could allow for wider usage. Either for that or for just a plotting function, plotting with polygons instead of with points for circles / squares would allow for resizing, and keeping the ratios and gaps between them the same.

jpquast commented 1 year ago

Hi Brady, thanks for your changes!

So you would just move {scales} to depends and not check for it since it is anyway included in {ggplot2}? I had it in suggests to decrease the number of dependencies. The check I had would anyway never be run since, as you say, {scales} is part of {ggplot2}. But since there are anyway not many dependencies I guess it is fine to have it there.

In that case you can also do the same with {farver} since the {scales} package depends on that (https://cran.r-project.org/web/packages/farver/index.html).

I indeed actually missed that I use the %>% pipe from {magrittr}. I guess we could just replace it with the base R pipe instead.

I am curious about the utils function you created for the pipe. What exactly is this for?

Regarding your suggestions! You are totally right! I think using a specific geom is likely the cleaner way to create this kind of plots. I have never looked into how one creates custom geoms. If you have some ideas please let me know!

Thanks a lot for you help!

BradyAJohnston commented 1 year ago

Pipes

The function for the pipe is just what is created when you run:

usethis::use_pipe()

Which adds everything to your project to use the pipe. Changing it all to base pipe would also work.

Imports

Regarding the imports, people get caught up too much with wanting to reduce dependencies. Because you are installing {ggplot2} you are already installing {scales} and {farver} then anyway. The number of dependencies are large already, they are just abstracted away behind other packages. You just need to explicitly declare them in the imports if you explicitly use any of their functions via:

scales::gradient_n_pal()

If you really wanted to reduce the number of dependencies, you could switch to base R the times you use tidyr or dplyr and remove those as dependencies. If you aren't aiming for it to be a core utility package that is then utilised by a bunch of other packages, it's not worth it, and the core tidyverse packages will certainly be well maintained for years to come.

Custom Geoms

I've made some custom geoms / stats before. I just followed the instructions on this page: https://ggplot2.tidyverse.org/articles/extending-ggplot2.html

It's a lot to get your head around initially, but worth it. I've used them in https://github.com/BradyAJohnston/plasmapR

Up to you whether it'd be worth setting up before sending to CRAN, would require a bit more development work to refactor some of the internals but would be worth it for extensibility.

jpquast commented 1 year ago

Great thanks! I didn't know about usethis::use_pipe(). But I think using the base pipe is better. Do you want to change it in the PR or should I do it after?

Regarding the dependencies, I am just used to trying to reduce the number of imported packages because of the protti package where we need to make sure to keep the number of dependencies low for it not to give the note about importing too many packages. But you are right, in this case it doesn't matter.

I will have a look at the custom geoms for a future version. I think I don't have enough time now to still change it.