Define cardinality traits #79
Conversation
| @@ -1,7 +1,7 @@ | |||
| name = "FeatureTransforms" | |||
| uuid = "8fd68953-04b8-4117-ac19-158bf6de9782" | |||
| authors = ["Invenia Technical Computing Corporation"] | |||
| version = "0.3.2" | |||
| version = "0.3.3-DEV" | |||
There was a problem hiding this comment.
I'm going to follow up with TestUtils before tagging a release
Codecov Report
@@ Coverage Diff @@
## main #79 +/- ##
==========================================
+ Coverage 99.10% 99.18% +0.07%
==========================================
Files 10 11 +1
Lines 112 122 +10
==========================================
+ Hits 111 121 +10
Misses 1 1
Continue to review full report at Codecov.
|
|
|
||
| Returns the [`Cardinality`](@ref) of the `transform`. | ||
| """ | ||
| function cardinality end |
There was a problem hiding this comment.
I initially thought defining cardinality on instances rather than types seemed redundant in a way. And that cardinality for each subtype of AbstractScaling seemed redundant. But I realised it's how traits are supposed to work right?
There was a problem hiding this comment.
I'm not sure I understand.
I could/should have defined the cardinality on AbstractScaling, which would have covered MeanStdScaling and IdentityScaling. In case another type of scaling comes along that's ManyToOne it would have to be special-cased in that instance. Is that what you're getting at?
There was a problem hiding this comment.
Yes that's partly what I was thinking. For some reason thought defining cardinality on AbstractScaling wouldn't work because it was an abstract type, so I didn't comment on that directly.
The other point I was making is, why should every instance of a given Transform subtype have this trait, when the trait is the same between all instances? Conceptually, cardinality belongs more with the type than the instances.
I backed off on that because I figured traits, as a design pattern, apply to instances. But I'm not even sure about that - is it a valid point?
There was a problem hiding this comment.
oh right, well they can apply to both.
You'll see I've changed the code so that cardinality now applies to AbstractScaling.
In that case it made more sense to attach it to the type because all concrete subtypes will have the same behaviour.
But I attached it directly to the concrete types of all other transforms because they don't have abstract supertype (besides Transform) to define it for. And if I defined it for Transform outright then users won't know they have to define it themselves.
There was a problem hiding this comment.
Yep that looks fine. Still not sure if I got this across though:
why should every instance of a given Transform subtype have this trait, when the trait is the same between all instances? Conceptually, cardinality belongs more with the type than the instances.
This means e.g.
julia> cardinality(T) where T <: Power = OneToOne()
julia> cardinality(Power)
OneToOne
vs.
julia> cardinality(::Power) = OneToOne()
julia> cardinality(Power(2))
OneToOne
First step towards #75 is to define the various traits and add them to the existing transforms