Internal settings container

[vincentarelbundock]

This is a more fleshed out example of the centralized internal settings strategy.

Again, no commitment, but I thought it would be useful to look at what it could actually look like. This currently works with just a named list (no S4), but there are 3-4 of the types have not been converted over yet.

@grantmcdermott @zeileis the file of interesting would be this one:

https://github.com/vincentarelbundock/tinyplot/blob/a913e12e45e593110293a663d201bdb247f94179/R/tinyplot.R

[vincentarelbundock]

CI failures are just warnings. Trivial stuff that would obviously need to be fixed, but the PR still works for illustration.

[grantmcdermott]

Thanks @vincentarelbundock. I'm a bit tied up over the next day or two, but will take a proper look at this as soon as I can.

[vincentarelbundock]

absolutely no rush at all

[zeileis]

Thanks, Vincent, for suggesting this. I like the approach with the simple settings list and the sanitize_*() functions which do list2env() and eventually modifyList(). This could work...but I didn't work through the details, yet. So far, I only have two more specific (and rather minor) comments:

The reason for including the raw_input settings in the settings themselves (https://github.com/vincentarelbundock/tinyplot/blob/internal_settings_v3/R/tinyplot.R#L735) is in order to have easy access to it in the sanitize_*() functions without requiring another argument? This feels a bit weird to me (and I don't like the name raw_input) but it's a very lean solution and I don't see a real argument against it.

The sanitize_ribbon.alpha() function is now a bit different from the other sanitize_*() functions because it does not take the whole settings as its argument. Also, it's called within setting up the original input settings (https://github.com/vincentarelbundock/tinyplot/blob/internal_settings_v3/R/tinyplot.R#L730) which differs from the other sanitizers.

[vincentarelbundock]

@zeileis two good points. agreed on both counts.

Ideally, I'd like to get rid of raw_input altogether, but I'll see if that's realistic.

And yes, a more consistent solution for ribbon alpha should be on my todo list.

[grantmcdermott]

Hi folks. I need to take a quick break from tinyplot to focus on other projects. Just want to check that you need specific feedback from me to continue iterating on this container workflow in my absence?

[vincentarelbundock]

No specific feedback. More looking for something like "yes I line the idea and it's ok to invest with relatively high likelihood of a merge (no guarantee, obv"

Or opposite :)

[grantmcdermott]

Ha, got it. In that case, yes, I think that this is a good direction to try out, especially given your parallel experiences with marginaleffects (even if we don't go the full S4 route). I'm optimistic that we would end up merging.

[grantmcdermott]

Thanks for this @vincentarelbundock... I finallly had time to review.

I've let a quite a few comments. Some of them are nits, some are clarifications, and some are a suggestions for potential improvements.

I have two high-level observations:

1. This container approach is cool, but I think we can tighten up on consistency and (hopefully) efficiency. One concern I have is the fact that we're basically lugging this settings item throughout the internal code, but "manually" reassigning certain elements each time, e.g. settings = update_settings(settings, list(x = x)). In that sense, it's really a glorified wrapper around modifyList (which is fine!). But perhaps we could use the fact that the target is always the same---i.e, settings---to tighten up the implementation so that the code becomes update_settings(x = x)? I don't know whether we should enable update by reference, but it could simplify the code further.

2. Following on from pt. 1, I'm a little uncomfortable with the fact that many of our internal functions now collapse down to taking a single settings argument, without explicitly making clear which settings are going to be extracted or adjusted. (I mean, you can obviously look inside the relevant function, but everything is more hidden.) I don't have an obviously better solution, but perhaps we can tweak the implementation so that instead of, say,

data_abline = function(settings, ...) {
    list2env(settings[c("datapoints", "lwd", "lty", "col")], environment())
...

we end up with

data_abline = function(params = c("datapoints", "lwd", "lty", "col"), ...) {
    list2env(settings[params], environment())
...

Or, if you wanted something fancier, you could extract the function args as strings:

data_abline = function(datapoints, lwd, lty, col), ...) {
    call = match.call()
    param_names = setdiff(names(formals()), "...")
    params = sapply(call[param_names], deparse1)
    list2env(settings[params], environment())
...

[vincentarelbundock]

@grantmcdermott those were some excellent comments! They lead me to re-think the approach, and I pushed a pretty important change. Now, what we are doing is:

1. settings is an environment.
2. Call env2env(settings, environment(), c("x", "y")) to make those two objects available here.
3. Call settings$x = x to update in-place, or...
4. Call env2env(environment(), settings, c("x", "y")) to update in-place more than one object.

This has three main advantages:

1. Since we are modifying the settings environment directly, we don't need to re-assign all the time like settings=update_settings(...)
2. IIUC, with this approach, we are only moving around references, rather than copying objects. If I'm right, then this should be much more efficient.
3. Downstream functions don't need to send settings back up. Then can just modify it directly.

Let me know if that's closer to what you were hoping for.

Edit: I know tests are failing. I'll look into it soon. Just wanted to explain the changes I just pushed.

[vincentarelbundock]

tests pass

[grantmcdermott]

I'm probably getting ahead of myself here, but am I correct in thinking that this new settings environment logic will allow us to to remove a lot of the stuff that we're currently shoehorning into the type_info escape hatch?

(I think you might have made this observation in your original proposal; so it's partly a reminder to myself.)

[vincentarelbundock]

100%. Could be a separate issue. I plan to do a bit more of this kind of work after this PR is merged.

[vincentarelbundock]

One crazy idea would be to push this until we reach delayed evaluation: #121

TBC, that is not my actual plan, and I'm not sure it's actually a good idea. But it could conceivably work.

[grantmcdermott]

This is fantastic @vincentarelbundock. The settings environment approach is exactly what I was looking for. I can see that this has already simplified (disciplined) a lot of the internal code, and I'm excited for some of the additional improvements that it will open up.

Do you mind adding a NEWS.md item? Everything else LGTM, so please feel free to merge once that's done..

[vincentarelbundock]

News item here: c701699