Custom types: Clarify how `name` impacts behavior, consider alternatives to hardcoded types in `tinyplot::tinyplot`

[katrinabrock]

Currently there are several places in the tinyplot::tinyplot function where there are different logic branches for different build-in types.

Examples:

https://github.com/grantmcdermott/tinyplot/blob/main/R/tinyplot.R#L1369-L1372

https://github.com/grantmcdermott/tinyplot/blob/main/R/tinyplot.R#L1176

There a couple usability issues with this:

1. It's not clear from the documentation exactly behavior is controlled by this name variable. In the vignette, it says "fallback behaviour same as "p" type (e.g., legend defaults)" and "think of it as a convenient way to inherit default/fallback behaviour from an existing plot type". But exactly what behavior is controlled by this param, and thus the consequences of choosing one built-in plot type over another is unclear. (For example, what are the consequences of choosing "l" vs "segments" if building a plot type where either could potentially fit?)
2. IMO It would be even nicer if the options that are currently controlled by these hard-coded type names in tinyplot::tinyplot could instead be controlled by the output of the type_x() function somehow. Maybe in the settings object? In the above examples, it looks like those options are whether empty plots are disallowed and the default x and y labels.

[katrinabrock]

cc @rhirk

[vincentarelbundock]

Sorry, but I don't understand your proposal. Could you maybe show pseudo-code for a tinyplot() call that you would like to make but can't, and describe the expected output?

[katrinabrock]

is point # 1 clear at least and only # 2 is unclear? or are they both unclear?

[vincentarelbundock]

TBH, both points are kind of fuzzy to me. But I just woke up, so this is definitely on me!

Re: 1

Are you just asking for better documentation? AFAICT, the only place in the code base that explicitly relies on type$name is the three lines you linked to, so this doesn't seem like a big deal. If you want to propose a reasonable change to the docs to reflect that, I bet Grant will consider it seriously.

Re: 2

Isn't that basically what we do already?

1. sanitize_type() accepts strings like "p" or "o" and maps it onto a type_*() function here: https://github.com/grantmcdermott/tinyplot/blob/main/R/sanitize_type.R
2. Functions like type_points() basically define all the (internal) parameters that control plots: https://github.com/grantmcdermott/tinyplot/blob/main/R/type_points.R

[katrinabrock]

Re 1.:

Yes, this is purely a documentation request.

There are more examples in deeper functions, like this one:
https://github.com/grantmcdermott/tinyplot/blob/main/R/facet.R#L281-L282

I could certainly attempt to track down all such examples and document them in a PR. It's a bit tricky for me though because I can't always quite tell what the code is actually doing.

Re 2.

I think you're right that an example here would help.

ok, take the example here:

https://github.com/grantmcdermott/tinyplot/blob/62463c0184c1fe12236f8d2dc3397a64a9dd9a7e/R/tinyplot.R#L1176C7-L1176C145

This is the code currently:

      empty_plot = FALSE
      if (isTRUE(empty) || isTRUE(type == "n") || ((length(ix) == 0) && !(type %in% c("histogram", "hist", "rect", "segments", "spineplot")))) {
        empty_plot = TRUE
      }

If I'm reading it correctly, default is that missing xi means empty plot. However, for c("histogram", "hist", "rect", "segments", "spineplot") missing xi does not mean empty plot (because these rely on xmin and friends). My proposal would be to change this line to something like:

      empty_plot = FALSE
      if (isTRUE(empty) || isTRUE(type == "n") || ((length(ix) == 0) && settings$missing_xi_means_empty_plot)) {
        empty_plot = TRUE
      }

and have the default for this missing_xi_means_empty_plot setting to be TRUE.

Then in the data_rect, data_hist, data_segments etc. You would add

settings$missing_xi_means_empty_plot = FALSE

The readability advantage is that someone comparing the function definitions type_segments and type_lines can clearly see that this is one of the differences in behavior between the two instead of having to search through the tinyplot code to discover it. The functional advantage is that someone creating a custom type that does not map neatly onto one existing type can still use a builtin's defaults, but then selectively override the behavior that doesn't align with the type they're building.

[vincentarelbundock]

[TBC, I'm not the main dev here, so these are all my personal reactions]

Re 1:

I agree that more documentation would be nice. IMO, documentation in man/ and on the website should be primarily user-facing, so we shouldn't document prominently corner cases that most users won't care about. That said, we could definitely improve developer-facing documentation to better explain how new types are defined.

Re 2:

It sounds like you want to remove type-dependent logic flags, and replace them by variables held in the settings environment, and defined by the relevant type_*() function.

In principle, I think that's a good idea, as it would make things more explicit and give more control to developers of custom type_*() function.

My main worries are:

• Proliferation of variables in settings can make things less readable.
• Premature optimization: There must be dozens (hundreds?) of type-specific decisions. Refactoring all of them would take a tremendous amount of time, and I expect that most of these changes would bring little actual benefit in practice.

Path forward?

I basically agree with your general recommendations, but the big (obvious) constraint is time. As a big, overarching project, this feels rather low priority to me, and I don't have a ton of time to invest on tinyplot as it is.

But if someone faces a very specific problem in a concrete context, I'd be happy to review a PR, or help implement a strategy to make type_*() clearer and more powerful.

[katrinabrock]

Yes, you have described my suggestion much more clearly than I did!

RE 1:

There is a whole section in the types vignette regarding custom types. That's where I would propose to add this documentation (or a new vignette similarly aimed at users who wish to make their own types).

RE 2:

Fair enough regarding time constraints.

Regarding proliferation of settings...they could be nested to make it more readable. Also, it doesn't necessarily have to be 1:1 settings with logic. The setting in my example would be a bit silly to have explicit. It seems like the existing logic is based on shared attributes among the existing types. For example, is x required or is the data really in xmin/xmax etc? There could be a setting like x_optional to capture this characteristic of types and that could be tested in multiple places.

Regarding a concrete example...I have run into some issues that sent me digging in this code. I haven't yet developed one that's "pure" enough to be a good test case/MRE. I will still try to construct one representing my use-case and post it here.