Skip to content

Reworking transforms for flexibility and fewer allocations#12

Open
scottmcm wants to merge 1 commit intorscarson:masterfrom
scottmcm:lots-of-transform-stuff
Open

Reworking transforms for flexibility and fewer allocations#12
scottmcm wants to merge 1 commit intorscarson:masterfrom
scottmcm:lots-of-transform-stuff

Conversation

@scottmcm
Copy link
Copy Markdown
Contributor

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::inverse method, 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:

  • Adding ∛ and √ transforms, which seem reasonable enough. (I also stuck non_exhaustive on the enum so more can be added later without another semver break.)
  • Figuring out what to do about ScaleTransform::Exponential, because its multiplicative factor can't be undone by Logarithmic's factor. (And, correspondingly, that ScaleTransform::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 Exponential and Logarithmic. 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 one T it's obvious. (An alternative here would be to move to something like Exponential { base: T, factor: T, phase: T for 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 combining Linear+Shift, etc. (See the example on inverse_array for a worked version of this.)

At first that went great, but then I realized that trying to use Transform::apply_to doesn't work at all for all the impure transformations. (I'd tried implementing apply by calling each transform's apply_to on each element, which is great for ScaleTransform but totally nonsensical for the smoothing ones and such.)

Thus the giant scary change here: letting Transform::apply iterate 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 complicated for<'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, MeanSubtraction can now iterate once to get the mean, then iterate again to subtract it. Strength::into_stddev no longer needs a slice, so its callers -- such as NoiseTransform::CorrelatedGaussian -- don't have to allocate. No more collecting into a Vec<&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 Transformable anyway 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:

  • Added XTransform and YTransform wrappers to adapt transforms from working on a scalar to working on a component of a pair.
  • Changed Transformable to taking a impl Transform instead of &impl Transform, but then adding a blanket impl<R: Transform> Transform for &R so that passing a reference to an impl Transform still works.
  • Switched ScaleTransform::Logarithmic to use T::min_positive_value() instead of T::epsilon() because there are a ton of floats smaller that epsilon. (For f64, ε is about 2e-16 and min_positive is about 2e-308.)
  • Removed the Self: Sized bound on Transformable::transformed so you can do things like array[i..j].transformed(…) (applying it to a slice) and it uses ToOwned to get you a Vec result.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant