Sorry forgot this never actually ended up getting pushed and it's my last day

In D89572#2337791, @rriddle wrote:

In D89572#2337652, @ahmedsabie wrote:

In D89572#2337635, @rriddle wrote:

In D89572#2337634, @ahmedsabie wrote:

In D89572#2335343, @rriddle wrote:

I don't really understand the motivation here, can you detail it a bit more. Why is the original trait not enough? Also, ValuePreserving seems too broad of a name for what this is doing.

This makes the original trait applicable in more places because it can ignore nodes that don't actually end up changing tensor values so you can still fold.

I don't understand this aspect. Is this supposed to mean that the operation would somehow mutate the input tensors? Putting the fact that tensors are value-typed aside, I would expect this to be something covered by side effects and not a trait like this. If an operation is going to mutate its input operands, that is something that should be expressed as a side effect.

No I mean if you had abs(permute(abs(x))) then that's the same as permute(abs(x)) but you can't generally apply this optimization to all side effect free operations, example abs(negate(abs(x))) is not the same as negate(abs(x)). ValuePreserving is capturing the property of permute that it doesn't actually produce new values when applied

Thank you for explaining, I understand what you mean by value preserving now. That being said, I don't think this optimization is legal with just idempotent and "value preserving". For example, stable_sort(permute(stable_sort(x))) is not strictly the same as permute(stable_sort(x)).

Update comment

Update name of trait and apply it to involutions instead

In D89572#2337635, @rriddle wrote:

In D89572#2337634, @ahmedsabie wrote:

In D89572#2335343, @rriddle wrote:

I don't really understand the motivation here, can you detail it a bit more. Why is the original trait not enough? Also, ValuePreserving seems too broad of a name for what this is doing.

This makes the original trait applicable in more places because it can ignore nodes that don't actually end up changing tensor values so you can still fold.

I don't understand this aspect. Is this supposed to mean that the operation would somehow mutate the input tensors? Putting the fact that tensors are value-typed aside, I would expect this to be something covered by side effects and not a trait like this. If an operation is going to mutate its input operands, that is something that should be expressed as a side effect.
No I mean if you had abs(permute(abs(x))) then that's the same as permute(abs(x)) but you can't generally apply this optimization to all side effect free operations, example abs(negate(abs(x))) is not the same as negate(abs(x)). ValuePreserving is capturing the property of permute that it doesn't actually produce new values when applied

In D89572#2335343, @rriddle wrote:

I don't really understand the motivation here, can you detail it a bit more. Why is the original trait not enough? Also, ValuePreserving seems too broad of a name for what this is doing.

Can you push it when you get a chance I don't have push access, thanks!

I dont have push access, if you get a chance can you push for me? Thanks!

Add the missing revert

In D88809#2320818, @mehdi_amini wrote:

Pushed a revert in 16580d791f3b ; can you try to build after configuring cmake with -DBUILD_SHARED_LIBS=ON ?

I don't have push access so if you get a chance to push my code and its good please do, thanks!

Fix last few minor comments

Allow traits to continue folding if an inplace fold occured

Added no side effect requirement to involution, changed fold to two different signatures to be consistent with existing fold functionality, and fixed issues addressed by comments

I agree with Mehdi on keeping it as one PR but updating the name since making a separate PR with just the trait folding would require writing code for its test that's almost the same as what's required for involution anyways

update commit message to be more descriptive of the infrastructure change