Skip to content

Increased transform flexibility#13

Merged
rscarson merged 1 commit into
rscarson:masterfrom
scottmcm:flexible-transforms
Apr 1, 2026
Merged

Increased transform flexibility#13
rscarson merged 1 commit into
rscarson:masterfrom
scottmcm:flexible-transforms

Conversation

@scottmcm
Copy link
Copy Markdown
Contributor

I think this bit of #12 is clearly worth doing, since it just makes things more flexible.

It seems obviously good to be able to .transform(…) slices too, not just Vec.

@rscarson
Copy link
Copy Markdown
Owner

First I want to say I really appreciate the work you've put into the project, but I will not be able to merge this as is, since it breaks two invariants of the trait:

  • Transforms should be reusable, so removing the &t will not work here
  • transformed specifically operates on a copy of the data - it is the clone variant of transform, which operates in place

However I think this does reveal some flaws in the transforms, namely:

  • We do need more flexible implementations, such as slices . Perhaps the trait should be split into in-place and copied traits
  • I also think this is fundamentally a failure of my documentation to adequately communicate these invariants

@scottmcm
Copy link
Copy Markdown
Contributor Author

  • transformed specifically operates on a copy of the data - it is the clone variant of transform, which operates in place

I'm not sure I follow this response -- that's what it still does, it just copies the data using .to_owned() instead of .clone(), so that you can "copy" the slice into a vec, rather than being restricted to Sized.

  • Transforms should be reusable, so removing the &t will not work here

Definitely curious to see what "reusable" means to you here. After all, the impl Transform for &impl Transform this added means you can always re-use a Transform in Transformable::transform by using a reference to it.

Comment thread src/transforms.rs
{
let mut new_data = self.clone();
new_data.transform(transform);
let mut new_data = self.to_owned();
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

annot: note specifically here that it's still making a copy of it https://doc.rust-lang.org/std/borrow/trait.ToOwned.html

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok fair point - I concede that the ToOwned version is more general.

However I still want to keep the &R here
implementing Transform for &Transform makes it compatible but imho encourages stateful or single use transforms, which I don't love

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me the thing that enforces that is that https://docs.rs/polyfit/latest/polyfit/transforms/trait.Transform.html#tymethod.apply takes &self, which I do not propose to change. You'd still have to use Cell or similar to make a stateful transform as a result.

I just got sad having to do blah.transform(&NormalizationTransform::MeanSubtraction); when the & didn't really seem to help anything at all, vs being able to just blah.transform(NormalizationTransform::MeanSubtraction);.

Comment thread README.md
// This will return an error!

let bad_prediction_probably = fit.as_polynomial().y(150.0); // This is outside the range of the data, but you asked for it specifically
let bad_prediction_probably = fit.as_polynomial().y(150.0); // This is outside the range of the data, but you asked for it specifically
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Readme is a generated file (cargo rdme), please don't directly alter it - also this instance is already gone

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did this with cargo rdme because the PR build failed originally without it.

@rscarson rscarson merged commit 21f83d3 into rscarson:master Apr 1, 2026
4 checks passed
@scottmcm scottmcm deleted the flexible-transforms branch April 2, 2026 18:23
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.

2 participants