CRAN submission

[grantmcdermott]

Tracking in this milestone: https://github.com/grantmcdermott/plot2/milestone/1

Now that facet support has been added, I want to address the main outstanding issues before CRAN submission. In my view, there are two key above-the-line issues that need to be resolved.

• Decide on the package name. #22
• Support continuous legends, if possible. #84 This would be a breaking change for certain cases, so I'd like to get it done before CRAN.

Other things I'd like to get done prior to a CRAN submission (but okay to add later):

• Histogram support. #72
• Reduce the size of the install tar file. #111

[vincentarelbundock]

To skip on CRAN, I set an environment variable on the GitHub workflow and on my computer. Then, at the top of the test script I check if the flag is set with Sys.getenv and call tinytest::exit_file() if not.

[mdsumner]

xplot ?

[harrelfe]

On a different topic I suggested implementation of general transformation functions rather than specific support for histogram and density, and provide a suite of basic transforms. On the package name I don’t personally like numbers in names because if sometimes implies planned obsolescence. Maybe Plot, bplot, or baseplot. Or something that implies “lean and efficient”.

[grantmcdermott]

@zeileis @vincentarelbundock I submitted to CRAN yesterday and received a request for changes, which I've pasted at the bottom of this comment (truncated to action items only).

Some of these are easy fixes or oversights (I could have sworn we reset par(op) during the examples...) But one or two will take more thinking. In particular, the hard requirement of on.exit(par(oldpar)) probably means that we will have rethink the whole par_restore and add logic.

One option is that, like we currently do for facets, we could capture the state of par before we exit and save it in an internal variable, which we can recall if add = TRUE. If we follow this approach, then the only change would be to enable this for every plot instead of just for facets. OTOH it does mean that users won't be able to add elements to an existing plot in the way that they may be used to with a “pure” base approach, e.g. using lines(), points(), etc. (C.f. The original issue thread raised @karoliskoncevicius by in #6.) But I don't really see a way around this anyway, given the CRAN requirements to reset par before exit.

...

Requested CRAN changes

Please do not start the description with "This package", package name, title or similar.

If there are references describing the methods in your package, please add these in the description field of your DESCRIPTION file in the form authors (year) authors (year) authors (year, ISBN:...) or if those are not available: with no space after 'doi:', 'arXiv:', 'https:' and angle brackets for auto-linking. (If you want to add a title as well please put it in quotes: "Title")

Please add \value to .Rd files regarding exported methods and explain the functions results in the documentation. Please write about the structure of the output (class) and also what the output means. (If a function does not return a value, please document that too, e.g. \value{No return value, called for side effects} or similar) Missing Rd-tags: draw_legend.Rd: \value tinyplot.Rd: \value tpar.Rd: \value

Please make sure that you do not change the user's options, par or working directory. If you really have to do so within functions, please ensure with an immediate call of on.exit() that the settings are reset when the function is exited. e.g.: ... oldpar <- par(no.readonly = TRUE) # code line i on.exit(par(oldpar)) # code line i + 1 ... par(mfrow=c(2,2)) # somewhere after ... e.g.: If you're not familiar with the function, please check ?on.exit. This function makes it possible to restore options before exiting a function even if the function breaks. Therefore it needs to be called immediately after the option change within a function.

Please always make sure to reset to user's options(), working directory or par() after you changed it in examples and vignettes and demos. e.g.: oldpar <- par(mfrow = c(1,2)) ... par(oldpar)

Please fix and resubmit.

[zeileis]

Let me investigate whether the par() restoration just pertains to the examples (which is clearly reasonable) or to all functions in the package (which would be overkill in my opinion). I think it should be possible to have functions that change the par() settings... I'll report back here.

[grantmcdermott]

Super, thanks @zeileis.

I've been struggling with how to incorporate this requirement into the code logic, especially since we call internal functions like draw_legend that change mar and oma on their own... but could have been passed already-changed par settings from higher up in the code (e.g. from our preceding facet logic).

For the record, and if we can't get an exemption, I think the simplest solution is just to call

opar = par(no.readonly = TRUE)
on.exit(par(opar), add = TRUE)

right at the top of the tinyplot.default function, instead of doing this every time an adjustment to par is made (which is what the CRAN email seemed to suggest).

[grantmcdermott]

PS. Adding CRAN submission changes in this branch https://github.com/grantmcdermott/tinyplot/tree/cran

I believe that I've addressed all of the requested changes, besides this (non-Examples) par issue that you're following up on for @zeileis.

The only other thing I can't figure out is the

If there are references describing the methods in your package, please add these in the description field of your DESCRIPTION file in the form...

bit. We don't have any reference in our Description , so I'm not sure where that is coming from.

[zeileis]

Regarding the references: In general, the CRAN team tries to encourage adding references to the DESCRIPTION files so that it is easier for users/readers to quickly find out what exactly a package does. For example, when a package implements a certain statistical model, it is very useful to have a link to the corresponding paper(s) in the description.

In the case of tinyplot I'm not sure though whether we can add references that would be really useful for the users. At the moment I haven't got a good idea. So at the moment I think that the best we can do is to (a) add a few more details/explanations in the description and (b) explain to the CRAN team that there is no specific reference we could add at the moment.

[grantmcdermott]

Sorry to bug @zeileis, but I'm guessing no updates from your CRAN contacts about the par restoration side effect yet?

[zeileis]

I've had some feedback but I'm still discussing. Will follow up here as soon as possible...

[zeileis]

OK, I discussed this a bit with Kurt. Apparently, there is no obvious precedent for this so we need to provide arguments why we want to things the way we want to do them. The following might work:

• tinyplot(..., par_restore = FALSE) remains the default and for tinyplot(..., par_restore = TRUE) we make sure that all parameters are really restored. I wasn't sure whether this is already fully implemented like that.

• Additionally, the original par() settings are stored somewhere so that one can do stuff like:

  tinyplot(...)

  annotation like points(), abline(), text(), etc.

  par(get_saved_par())

  Again I'm not sure what needs to be done in addition to what we already have. The .tpar environment is already partially used for this but maybe that could be extended and streamlined.

• The examples then need to make sure that we always restore the original par() settings. Ideally the different mechanism for achieving this could be shown somewhere.
• And then we can (briefly) explain to the CRAN team that we now provide different mechanisms for restoring par() settings.

I also had a look at the internals of .tpar but didn't fully get why this is implemented like this. Personally, I would have just used something like this:

.tinyplot_env <- new.env()
get_tpar <- function() return(.tinyplot_env$tpar)
set_tpar <- function(...) {
   dots <- list(...)
   if(!is.null(.tinyplot_env$tpar)) dots <- modifyList(.tinyplot_env$tpar, dots)
  .tinyplot_env$tpar <- dots
  invisible(NULL)
}

And in .onLoad the set_tpar() function could be called. Then the environment is always there and just gets updated when loading the package. And in addition to tpar we could store the par settings and potentially other stuff there.

But maybe I'm overlooking some subtleties here...

[grantmcdermott]

Great, thanks @zeileis. I agree with all of your suggestions. (As you imply, we already do some of these but we can make them more explicit in the examples.) My weekend has been a bit packed, so I don’t know if I’ll be able to action these steps by Monday. But I’ll try to start working on them asap.

PS. One annoying UX inconsistency that I meant to fix a while back is that par_restore (and ribbon_alpha) snake_case, whereas all the other tinyplot arguments are dot.case. So I’ll aim to address that as well here.

PPS. I need a bit more time to respond to your .tpar implementation query. But have run now so will try to address/explain when I have more time.

[zeileis]

I didn't notice the inconsistency up to now - probably because I'm so used to dot.case in par() that I didn't expect anything else in tpar(). And in tinyplot() I think I only actively used the snake_case arguments. But I think you are right and dot.case is probably the best way forward here because it is most similar to base R plots.

Let me know if I should try to help with anything else.

[grantmcdermott]

Okay, I think that I've just about managed to address all of the outstanding issues. Again, all changes are tracking in the "cran" branch here: https://github.com/grantmcdermott/tinyplot/tree/cran

Some notes:

• All examples in the documentation now ensure that the user's original par settings are restored properly. There was a typo in my original submission where I missed doing this correctly in one case, but that's now fixed.
• I've tried to expand and update the documentation, giving more details about the rationale for our persistent par settings (i.e., for adding annotations), as well as showing different options for restoring the user's original settings. Speaking of which...
• I've added a new get_orig_par() function, in the spirit of your suggestions @zeileis. Here's an example from this function's help documentation:

pkgload::load_all("~/Documents/Projects/tinyplot")
#> ℹ Loading tinyplot

# Contrived example where we draw a grouped scatterplot with a legend and
# manually add corresponding best fit lines for each group
tinyplot(Sepal.Length ~ Petal.Length | Species, iris)
for (s in levels(iris$Species)) {
  abline(
    lm(Sepal.Length ~ Petal.Length, iris, subset = Species==s),
    col = which(levels(iris$Species)==s)
  )
}

# Reset the original parameters (here: affecting the plot margins) and draw
# a regular plot
par(get_orig_par())
plot(1:10)

What's left to do?

@zeileis Did I miss anything? One thing I haven't done yet is fleshed out the package Description to sidestep the references issue. It's late this side and I'm tired, but I can look again tomorrow. Otherwise I'll gladly take any suggestions. (Could we just explain it as part of our CRAN submission note?) Similarly, I'd be grateful if you could suggest any text that I should include from your conversation with Kurt as part of the submission note. Happy to jump on a Zoom call to discuss if that's easier.

[grantmcdermott]

P.S.

I also had a look at the internals of .tpar but didn't fully get why this is implemented like this. Personally, I would have just used something like this:

The short version is that I based my implementation on something that I'd seen @SebKrantz do for his very nice collapse package here. (Seb, sorry to tag you out of the blue; feel free to unsubscribe.) The further background is that I'd seen Sebastian make a convincing case that this implementation allows you to combine (a) setting package options(...) that a user can call at load time (if set), with (b) a way of setting/getting package options on the fly that minimizes overhead.

All that being said, I might have overcomplicated things in my tpar adaptation, especially in further trying to further mimic the special behaviour of par (e.g. here). Thus, I'm certainly open to simplifying. But right now my feeling is that everything works, so it's probably best to leave as-is and revisit once this CRAN submission goes through.

[zeileis]

Grant, thanks for the update. Regarding the suggested get_saved_par() and your implemented get_orig_par(): I hadn't explained the idea well enough. What you implemented gets the par() settings from before the last tinyplot() call. But what I had in mind was to get the par() settings from the last tinyplot() call. This should enable doing something like this:

tinyplot(Sepal.Length ~ Petal.Length | Species, data = iris, restore.par = TRUE)
par(get_saved_par())
for (s in levels(iris$Species)) {
  abline(
    lm(Sepal.Length ~ Petal.Length, iris, subset = Species==s),
    col = which(levels(iris$Species)==s)
  )
}

Then users could set restore.par = TRUE routinely - maybe even via tpar() - but they would still be able to add graphical elements afterwards.

[zeileis]

Two comments regarding the tpar() environment:

• Delaying any work on this until after the CRAN release is fine. The change should not affect the end users.
• Is anything easier with your current implementation that would not be possible with a simple environment that is always shipped within the package namespace?

[grantmcdermott]

Then users could set restore.par = TRUE routinely - maybe even via tpar() - but they would still be able to add graphical elements afterwards.

Ah, I see. We effectively do this already for adding to facet plots via the get_last_facet_par helper function (see setter and getter calls here and here). So it would be a simple matter of enabling for all plot types, not just facets.

Stepping back, perhaps we should roll this dual functionality into a single get_saved_par(from = c("before", "after")) function, where:

• get_saved_par("before") (the default) would retrieve the saved par settings from the start of the most recent tinyplot() call, i.e., before any additional changes.
• get_saved_par("after") would retrieve the saved par settings from the end of the most recent tinyplot() call, i.e., after and inclusive of any additional changes.

Does that sound right? Let me know if you would change the default to "after" to be more consistent with your original idea.

P.S. I like also like your suggestion for users to enable/change the default restoration behaviour via tpar(restore.par = <logical>). Let me look into an implementation for this too.

[zeileis]

I have a slight preference for separate function names... but maybe I have been getting too used to the tools naming style for technical helper functions a la file_path_as_absolute etc. 🙈

The long explicit names avoid the risk of having the wrong memory or expectation about the default.

I'm also not sure what the more intuitive default would be or what more users would expect, possibly "start". Personally I would probably always set the argument explicitly.

[grantmcdermott]

Sorry for the radio silence. Very busy with work and family visiting.

I've gone ahead with my suggested implementation of get_saved_par() in https://github.com/grantmcdermott/tinyplot/pull/151/commits/3d57e7d1326a676d54b7d002d2e2af111f96d52a, which allows users to recover the par settings from either immediately before or after the preceding tinyplot() call. Apologies for not breaking them out into separate arguments per your request @zeileis, but I'd already begun this implementation and I quite like the generality, so am using my weak veto power :-)

At any rate, the help documentation of this new get_saved_par() function is quite detailed and should go some ways to satisfying CRAN's requirements. (I hope). Together with tinyplot(..., restore.par = TRUE) andtpar() (and even dev.off), I feel like we've given plenty of options for users to control and recover their par() settings.

The only remaining issues before attempting a CRAN re-submission are:

• [x] Do we need any additional documentation on how/why tinyplot adjusts a user's par settings (and how users can control/revert this behaviour)? I guess I could write a short vignette about it, but seeing as we exclude vignettes from the CRAN submission anyway due to size issues, this strikes me as overkill with non-marginal effort.
  • [x] @zeileis Related: Do you mind if I reference your conversation with Kurt during this re-submission to explain all the steps we took given the lack of precedence?
• [x] I still need to add a reference to the Description (or explain the lack thereof). I'll do so tonight.

[zeileis]

Thanks for all the work and updates! Quick comments:

• I agree that an additional vignette would be overkill. I think that the additional documentation for get_saved_par() should already be sufficient.
• You can mention it but I wouldn't over-emphasize it. The solution needs to be convincing enough - no matter who suggested it.
• I think that an explanation why there is no reference should be ok.

[grantmcdermott]

Just resubmitted. I tried to document/summarise the main points in cran-comments.md. Let's hold thumbs!

[grantmcdermott]

Accepted and now available on CRAN 🎉🎉🎉 https://cran.r-project.org/web/packages/tinyplot/index.html

Thanks @zeileis and @vincentarelbundock for all your help in getting this over the line!

[zeileis]

:partying_face:

Thank you for all your work on this, Grant! I'm very happy that this is officially out on CRAN and that I was able to help a tiny bit.