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

Add package scope page #44

Closed
wants to merge 15 commits into from
Closed

Add package scope page #44

wants to merge 15 commits into from

Conversation

joshwlambert
Copy link
Member

This PR adds a package scope page to the blueprints document. This page contains information around the use of plotting functions, plotting in vignettes and dependencies.

This PR relates to #7.

@joshwlambert joshwlambert added the documentation Improvements or additions to documentation label Jun 8, 2023
package-scope.qmd Outdated Show resolved Hide resolved
package-scope.qmd Outdated Show resolved Hide resolved
package-scope.qmd Outdated Show resolved Hide resolved
@joshwlambert joshwlambert marked this pull request as ready for review June 12, 2023 15:29
package-scope.qmd Outdated Show resolved Hide resolved
Copy link
Member

@Bisaloo Bisaloo left a comment

Choose a reason for hiding this comment

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

Thanks for consolidating the discussion into an actual blueprint page!

I think I would frame it differently though: the question is not so much base vs ggplot, or related to dependency issues, but to the fact that it's hard to provide custom yet generalizable plotting functionality.

This is why I like the geom / scale option for example because it provides tools for users to build their own plots, but doesn't lock them in an unflexible framework.

package-scope.qmd Outdated Show resolved Hide resolved
package-scope.qmd Outdated Show resolved Hide resolved
@Bisaloo
Copy link
Member

Bisaloo commented Jun 22, 2023

As a reminder for when I start working on this, this post reflects exactly my experience in other packages, and summarizes what I explained above:

https://mastodon.social/@coolbutuseless@fosstodon.org/110587094854201080

I have been down paths of creating custom plots with lots of arguments. It got painful quickly, as it diverged from what people already understood with {ggplot2}.

Instead I created a few custom geoms/stats, then it was just a case of telling people:

"You know ggplot. Here are our in-house custom geoms. Facets and everything else work the same. Go for it!"

This was referenced Feb 12, 2024
Copy link

netlify bot commented Mar 21, 2024

Deploy Preview for playful-gelato-7892ba ready!

Name Link
🔨 Latest commit 9feea8c
🔍 Latest deploy log https://app.netlify.com/sites/playful-gelato-7892ba/deploys/660bc3dabe6e140008afd59e
😎 Deploy Preview https://deploy-preview-44--playful-gelato-7892ba.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@chartgerink
Copy link
Member

chartgerink commented Mar 21, 2024

Hi - @joshwlambert and I worked on substantially revising this page and re-requested reviews on the new version. Would be great if you could have a look at the new and improved page for the plotting functionality.

PS: I'll fix the markdownlint of course :-)

Copy link
Member

@Bisaloo Bisaloo left a comment

Choose a reason for hiding this comment

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

Thanks for your work on this!

I like the conciseness and the clarity of the text but I think the main point, which is what I tried to explain in my previous comments, is still buried in a single sentence in the middle of the text.

The main problem with ggplot2 plots is not dependencies. ggplot2 is already installed on nearly all machines and it's today a reasonably lightweight package. Due to its very large number of reverse dependencies, ggplot2 maintainers have to be very careful with breaking changes and it's generally a safe and good candidate for a dependency IMO.
On this note, do note that providing geoms still require to Import ggplot, which undermines the point about avoiding taking it as a dependency.

The main problem is that from my experience, and others' experience, it's pretty difficult (impossible?) to beat ggplot2 on the complexity / flexibility trade-off. The layering system is a very convenient way to produce infinitely customizable plots, and it is the syntax users are used to. When attempting to wrap ggplot functionality, you lost this layering syntax and end up with either a completely unflexible function, or a function with dozens or even hundreds of arguments, which is not user-friendly or maintainable.

wrap_ggplot <- function(data, linewidth, colour, linetype, xlab, ylab, title, subtitle, caption_name, facets, wrap_or_grid, theme, ...) {

} 

This is why we are recommending geoms. We are still importing ggplot2 and we are fine with it. But we are keeping the clever and flexible layering system which makes the strength of ggplot. We only focus on how the data is converted to a ggplot object / layer. Any aspects not directly related to this (colour scale, lab names, etc.) are left to the user.

The vignette approach is a simpler way to achieve the same result. We are still using the layering system and let the user copy/paste/edit the relevant layers to address their needs.

On the point about base plots, we don't have this dilemma with base plots because they are not using a layering system anyways. Wrapping them does not fundamentally change their syntax or their customisability.

plotting-functionality.qmd Outdated Show resolved Hide resolved
plotting-functionality.qmd Outdated Show resolved Hide resolved
@Bisaloo
Copy link
Member

Bisaloo commented Mar 21, 2024

Before I forget: we need to add @chartgerink in the CONTRIBUTORS.txt list before merging this PR.

@chartgerink
Copy link
Member

Thanks @Bisaloo - will revise after @TimTaylor indicates their points as well. I tried to incorporate your explanation without it becoming an exposé on the technical details, but I understand I cut out a bit too much 😅 I'll retry to find the balance.

@Bisaloo
Copy link
Member

Bisaloo commented Mar 21, 2024

Thanks @Bisaloo - will revise after @TimTaylor indicates their points as well. I tried to incorporate your explanation without it becoming an exposé on the technical details, but I understand I cut out a bit too much 😅 I'll retry to find the balance.

Yes, that's a fine line. A couple of thoughts on this:

  • we could always have a blog post we link to for technical details
  • if you feel the technical details on complexity vs flexibility, layering, etc. should be kept short here, balance can always be restored by making the entire page shorter. The fact we spend most of it on dependencies when it's a weak (and kind of unfair to ggplot2 dev) point is what is creating the lack of balance.

chartgerink and others added 2 commits April 2, 2024 10:30
Co-authored-by: Hugo Gruson <10783929+Bisaloo@users.noreply.github.com>
@chartgerink
Copy link
Member

I am not sure how to resolve the final issues at the moment. This PR has been open for 10 months, so I started working on it in an attempt to close it out. When a PR/issue is open for this long my experience tells me it is unlikely to be merged/resolved for variable reasons (e.g., context changes, unsatisfying results, open discussion points).

As a result, I now am tending towards cutting the reasoning+alternatives bits to balance out the page (they might also work better in a blog post as @Bisaloo suggested). Once cut, the whole page is <10 lines and as such (potentially) does not really warrant an entire page anymore.

I also notice I get the gist of the technical discussion but writing for it in a balanced manner is more difficult for me 😅 I wonder what best to do at the moment to get this to a mergeable state, because I suspect if we do not do this now this PR will simply go stale and not get merged at all.

@TimTaylor
Copy link
Contributor

  • Agree with @Bisaloo that the dependency focus should be dropped (also note it seems that you already have a whole page on dependencies in the doc).
  • I wouldn't worry about the brevity as think a short and sweet page (e.g. "A note on plot functionality") is OK.
  • I'd be inclined to start a new PR: . It's too hard to follow at this size and looks like the focus is narrower than when it was started (title being "Add Package scope page").
  • Main gist to me is:
    • plotting is subjective.
    • {ggplot2} is so flexible that you want to augment rather than wrap it.
    • Emphasise the providing of example code in vignettes / turorials.

@chartgerink
Copy link
Member

Okay, it seems like this pull request isn't going anywhere. It's been stale for over a month with no clear path to merging.

To prevent more time being sunk into this, I am closing it until the need arises again to address this issue.

@chartgerink chartgerink closed this May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants