RFC: Composite transform
#108
Conversation
Codecov Report
@@ Coverage Diff @@
## main #108 +/- ##
==========================================
+ Coverage 98.88% 99.51% +0.63%
==========================================
Files 15 16 +1
Lines 179 208 +29
==========================================
+ Hits 177 207 +30
+ Misses 2 1 -1
Continue to review full report at Codecov.
|
| The transforms in `Composite([t1, t2, t3])` are applied in `t1`, `t2`, `t3` order, where | ||
| the output of `t1` is the input to `t2` etc. When using `∘` to create transforms, the order | ||
| is `t3 ∘ t2 ∘ t1`, as in function composition. |
There was a problem hiding this comment.
hmm...that might get confusing...
There was a problem hiding this comment.
Yeah I had it the other way around first but the issue then is that the c.transforms[1] is the last transform that is applied, which is even more confusing I think.
The only totally non-confusing way is to get rid of the ∘ syntactic sugar, which is what switches the order.
There was a problem hiding this comment.
The only totally non-confusing way is to get rid of the ∘ syntactic sugar, which is what switches the order.
I wouldn't be against this tbh, but it might be handy for building pipelines of transforms like
tc = Composite(t1, t2)
...
...
tc = t3 ∘ tc
...
tc = t4 ∘ tcso I can see value in it being used that way
| @@ -46,3 +46,13 @@ struct ManyToMany <: Cardinality end | |||
| Returns the [`Cardinality`](@ref) of the `transform`. | |||
| """ | |||
| function cardinality end | |||
|
|
|||
| Base.:(∘)(::OneToOne, ::OneToOne) = OneToOne() | |||
There was a problem hiding this comment.
I'm not sure if ∘ is the right syntax here... it's used to compose functions whereas here we're sort of "reducing" over the cardinalities.
I guess it makes sense in the context of composing the equivalent transforms?
If we do go for it then ∘ becomes part of the Cardinality API and should be documented.
There was a problem hiding this comment.
The alternative is to have an internal _compose function which would do this instead? I don't think users need to use this at all.
There was a problem hiding this comment.
yeah making it internal would indicate it shouldn't be used publically
| @test_throws ArgumentError id ∘ LinearCombination([1, 2, 3]) | ||
| @test_throws ArgumentError OneHotEncoding([1, 2]) ∘ id |
There was a problem hiding this comment.
alternatively we could use test_skip until these are implemented?
There was a problem hiding this comment.
I wanted to make sure there is an actual error, so possibly @test_broken is the right thing
Co-authored-by: Glenn Moynihan <[email protected]>
Closes #105
In the present form, it is limited to
OneToOne()component transforms, which have the same keyword arguments when callingapply.In order to generalise
Compositetransforms the keyword arguments should become a part of the constructor rather than being passed in theapplycall.Todo: