Reworking transforms for flexibility and fewer allocations#12
Open
scottmcm wants to merge 1 commit intorscarson:masterfrom
Open
Reworking transforms for flexibility and fewer allocations#12scottmcm wants to merge 1 commit intorscarson:masterfrom
scottmcm wants to merge 1 commit intorscarson:masterfrom
Conversation
7bb1d2b to
48a2660
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Opening this probably more as a discussion, because while I started trying to do something simple, I ended up way down a rabbit hole and this is really bigger than probably makes sense to land as one thing, but figured I'd open it with all the stuff and we can chat about whether you even want all these changes and how to split them into smaller steps.
I started just trying to make a
ScaleTransform::inversemethod, because after scaling the inputs to something I often wanted to un-scale the outputs, and I'm really good at getting that inverse wrong.That required two changes:
non_exhaustiveon the enum so more can be added later without another semver break.)ScaleTransform::Exponential, because its multiplicative factor can't be undone byLogarithmic's factor. (And, correspondingly, thatScaleTransform::Logarithmic's factor is kinda weird since it could always be collapsed into the base.)So the first possibly-controversial change: I removed the factors from
ExponentialandLogarithmic. TBH, I find that kinda reasonable anyway, because I always thought that(T, T)was quite unclear for exactly what they were doing, whereas with just oneTit's obvious. (An alternative here would be to move to something likeExponential { base: T, factor: T, phase: Tfor x' = factor * pow(base, x - phase), but that felt kinda like overkill.) I also like that that makes the size of the variants more consistent.To make sure that that was still possible, I had the idea that ended up being the cause of all my problems: let people put transforms in an array to compose them so that we don't need all the pre-composed versions. That way the migration path from
ScaleTransform::Exponential(b, f)is to[ScaleTransform::Exponential(b), ScaleTransform::Linear(f)]. And it also opens the door to affine transformations by combiningLinear+Shift, etc. (See the example oninverse_arrayfor a worked version of this.)At first that went great, but then I realized that trying to use
Transform::apply_todoesn't work at all for all the impure transformations. (I'd tried implementingapplyby calling each transform'sapply_toon each element, which is great forScaleTransformbut totally nonsensical for the smoothing ones and such.)Thus the giant scary change here: letting
Transform::applyiterate multiple times.That definitely comes at a cost, because the signature needs to change from the easy
impl Iterator<Item = &mut T>to the more complicatedfor<'a> &'a mut I: IntoIterator<Item = &'a mut T>. The compiler definitely isn't as comfortable dealing with ∀'a bounds as it is for more "normal" things. If it was just for this array case, that wouldn't be worth it at all.But one cool consequence of it is that a bunch of transforms no longer allocate! For example,
MeanSubtractioncan now iterate once to get the mean, then iterate again to subtract it.Strength::into_stddevno longer needs a slice, so its callers -- such asNoiseTransform::CorrelatedGaussian-- don't have to allocate. No more collecting into aVec<&mut T>. And it's all still safe code, too.So that seems overall at least plausible, since so long as one normally goes through
Transformableanyway you don't even see the difference; things just work better under the hood. And thus this PR 🙂A few other notes of things I did along the way:
XTransformandYTransformwrappers to adapt transforms from working on a scalar to working on a component of a pair.Transformableto taking aimpl Transforminstead of&impl Transform, but then adding a blanketimpl<R: Transform> Transform for &Rso that passing a reference to animpl Transformstill works.ScaleTransform::Logarithmicto useT::min_positive_value()instead ofT::epsilon()because there are a ton of floats smaller thatepsilon. (Forf64, ε is about 2e-16 and min_positive is about 2e-308.)Self: Sizedbound onTransformable::transformedso you can do things likearray[i..j].transformed(…)(applying it to a slice) and it usesToOwnedto get you aVecresult.