[variations] Add support for extrapolation#1697
Conversation
| /// | ||
| /// Specifically, the `axis_ranges` argument implies extrapolation: it is | ||
| /// a map of the allowed ranges for input values. If it is `None`, then we | ||
| /// will only allow values in the range `-1.0..=1.0`. |
There was a problem hiding this comment.
This is a bit weird and non-obvious and makes for weird calls with , None. Lets just have a different method to compute with extrapolation? Or just have the model you create with new_extrapolating always do it, keep the signature of scalar_at?
There was a problem hiding this comment.
so i tried that out first but it felt weird: there are only two calls to this method (outside of tests) so it felt like an additional complication to add an extra method just for that?
There was a problem hiding this comment.
Those two calls, and all the test calls, go from being relatively intuitive to wtf is the , None.
Thinking "aloud,"
We are already constructing an explicitly extrapolating model, why can't it capture enough to do the right thing in scalar_at without changing the signature?
If that won't work lets just add a different entry point since presumably the caller explicitly knows they want a scalar with extrapolation?
If even that won't work for some reason then I suggest we use an enum other than Option, e.g. scalar_at(loc, Extrapolation::Off) vs scalar_at(loc, Extrapolation::On(range) to mitigate the wtf is None issue.
rsheeter
left a comment
There was a problem hiding this comment.
LGTM subject to some sort of change to address scalar_at
This is necessary in order for smart components to work correctly.
e99511f to
f3ad865
Compare
This adds the ability for us to handle extrapolation in the variation model, which is used by smart components.
(on top of #1696)