JuliaML / TableTransforms.jl

Transforms and pipelines with tabular data in Julia
https://juliaml.github.io/TableTransforms.jl/stable
MIT License
103 stars 15 forks source link

Use inverse instead of Base.inv to invert transforms #221

Closed ParadaCarleton closed 9 months ago

ParadaCarleton commented 10 months ago

See here; it would be great to have support for the InverseFunctions.jl, to make sure we have compatibility with invertible functions from other packages!

juliohm commented 10 months ago

Are you aware that we already have the notion of revertibility and invertibility implemented? Check the traits isinvertible and isrevertible in TransformsBase.jl. Whenever a transform isinvertible, we implement the inv function. Examples are geometric transforms in Meshes.jl such as Translate and Rotate.

ParadaCarleton commented 10 months ago

Are you aware that we already have the notion of revertibility and invertibility implemented? Check the traits isinvertible and isrevertible in TransformsBase.jl. Whenever a transform isinvertible, we implement the inv function. Examples are geometric transforms in Meshes.jl such as Translate and Rotate.

Right--what I'm saying is that in this case, the invertibility/revertibility interface happens to be the same as the one used by InverseFunctions.jl, so we can avoid code duplication by sharing (and also support more functions in InverseFunctions.jl).

juliohm commented 10 months ago

I don't think we need the additional functionality of the package. Base.inv together with the isinvertible trait covers all our needs. Also, revertibility is not the same as invertibility, and it is not covered in InverseFunctions.jl.

Thank you for the suggestion.

ParadaCarleton commented 9 months ago

Also, revertibility is not the same as invertibility, and it is not covered in InverseFunctions.jl.

I'd be happy to add this feature to InverseFunctions.jl, and it's been asked for before. (I assume by "revertibility" you mean left inverses?)

I don't think we need the additional functionality of the package. Base.inv together with the isinvertible trait covers all our needs.

The problem is this doesn't allow for extensibility or cases that aren't already hard-coded into TableTransforms.jl, and hard-coding is the kind of thing that often comes back to bite you. For instance, if I create an inverse-hyperbolic-sine transformation and try to use it with TableTransforms.jl, I'll find that they're not compatible, even if IHS is compatible with the standardized interface for this.

juliohm commented 9 months ago

I'd be happy to add this feature to InverseFunctions.jl, and it's been asked for before. (I assume by "revertibility" you mean left inverses?)

No, it means that one can undo operations even if there are losses. PCA projects the table onto a basis, and can "unproject" even if some variance is lost.

The problem is this doesn't allow for extensibility or cases that aren't already hard-coded into TableTransforms.jl, and hard-coding is the kind of thing that often comes back to bite you.

The problem is that Base.inv is the Julia way of expressing invertibility, and it is builtin to the language. We don't want extra dependencies to do what we can already do with a fresh Julia install. Also, we are building a cohesive stack on top of a well defined interface in TransformsBase.jl. If you seek integration, then you should probably adopt this package as a dependency as opposed to us adding extra packages that cover similar traits.

ParadaCarleton commented 9 months ago

The problem is that Base.inv is the Julia way of expressing invertibility,

Base.inv is meant for calculating multiplicative inverses (i.e. 1/x), rather than general inverse functions. inverse has a different name to avoid causing any confusion here. (We want something like inverse(Float) to error instead of silently propagating .)

We don't want extra dependencies to do what we can already do with a fresh Julia install. The package is an interface package, so it's a very small dependency (effectively 0 compile time, and basically the same length as implementing it by hand in TableTransforms.jl).

If you seek integration, then you should probably adopt this package as a dependency as opposed to us adding extra packages that cover similar traits.

This probably wouldn't make sense, since TableTransforms.jl is a very big package (while InverseFunctions.jl is a very minimal interface, designed to be lightweight so other packages can depend on it).

juliohm commented 9 months ago

This probably wouldn't make sense, since TableTransforms.jl is a very big package (while InverseFunctions.jl is a very minimal interface, designed to be lightweight so other packages can depend on it).

TableTransforms.jl != TransformsBase.jl

Also, if you feel that the name Base.inv should not be the one adopted in TransformsBase.jl, then feel free to submit a PR there with the ideas from InverseFunctions.jl. That is the most likely path to get the features you need into the stack we are building.

ParadaCarleton commented 9 months ago

That is the most likely path to get the features you need into the stack we are building.

The feature I really need is the ability to use functions from InverseFunctions.jl. Apart from that, we want to avoid type piracy on Base.inv, which is a frequent source of headaches and bugs.

If you're concerned about the implementation details, I can copy-paste all the code for invertible functions from TransformsBase.jl into InverseFunctions.jl, then just rename inv to inverse. I basically just want to split this interface off from TransformsBase (to make it a smaller dependency and let others use it).

juliohm commented 9 months ago

Can you please clarify with a practical example which functions of InverseFunctions.jl do you need? What features exactly are missing in TransformsBase.jl?

juliohm commented 9 months ago

I was chatting with @eliascarv and he reminded me that we already use the name inverse specifically for the Functional transform here: https://github.com/JuliaML/TableTransforms.jl/blob/84453f973a3203d1ee0ca0beba65ac3348c6a1ec/src/transforms/functional.jl#L35-L46

These are inverses of mathematical functions, not transforms. Is that what you are trying to achieve with this issue @ParadaCarleton?

ParadaCarleton commented 9 months ago

Yep, exactly that!

Can you please clarify with a practical example which functions of InverseFunctions.jl do you need?

Right now, I mostly just want to make one PR dealing with InverseFunctions.jl and TableTransforms.jl when I'm implementing additional invertible functions (inverse trig+inverse hyperbolic trig especially).

juliohm commented 9 months ago

@ParadaCarleton in order to fix this issue, we need to add missing features in InverseFunctions.jl:

  1. A trait function isinvertible that tells whether or not a function (or transform) is invertible
  2. Default inverse implementations for bulit-in trigonometric functions listed above

Can you help submitting a PR to InverseFunctions.jl with these additions? After that we can add the package as a dependency in TransformsBase.jl and update downstream packages accordingly.

ParadaCarleton commented 9 months ago
  • A trait function isinvertible that tells whether or not a function (or transform) is invertible

I think this is , but for now the way to signal this is with NoInverse. Calling inverse(f) will return either the inverse of f (if it exists), or NoInverse if it doesn't. Then, trying to call f_inv = inverse(f); f_inv(x) will throw an error; or you can add an inner function that dispatches on the type of f_inv to handle this case differently if you'd like.

ParadaCarleton commented 9 months ago

2. Default inverse implementations for bulit-in trigonometric functions listed above

By the way, you can use setinverse for now for any tests or development, while I'm working on adding left and right inverses.

juliohm commented 9 months ago

https://github.com/JuliaML/TableTransforms.jl/pull/227 fixes this issue.

We are waiting for your cos/sin additions to InverseFunctions.jl @ParadaCarleton

juliohm commented 9 months ago

A patch release is now available with this fix.