-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor tests to test types and transform functionality separately #84
Conversation
c50301e
to
a8c8616
Compare
Codecov Report
@@ Coverage Diff @@
## main #84 +/- ##
==========================================
- Coverage 99.25% 99.25% -0.01%
==========================================
Files 12 12
Lines 135 134 -1
==========================================
- Hits 134 133 -1
Misses 1 1
Continue to review full report at Codecov.
|
c56a1a1
to
0d2e038
Compare
Not sure why tests are failing considering #93 (which is branched from this) are passing ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Nice job. -1000+ lines 🤤
|
||
@testset "ManyToOne" begin | ||
T = FakeManyToOneTransform() | ||
@test FeatureTransforms.apply(x, T; dims=1) == ones(3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does a ManyToOne
transform work without dims
as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nope, _preformat
expects it not to be Colon()
julia> T = FakeManyToOneTransform()
FakeManyToOneTransform()
julia> A = ones(4, 3);
julia> T(A)
ERROR: MethodError: no method matching length(::Colon)
|
||
@testset "OneToOne" begin | ||
T = FakeOneToOneTransform() | ||
@test FeatureTransforms.apply(x, T) == ones(2, 3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These transforms don't preserve type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it does in general, and reminder that
julia> KA = KeyedArray(ones(3, 5), features=[:a, :b, :c], time=1:5);
julia> KA == ones(3, 5)
but as usual AxisArrays breaks the mould, and any operation on them doesn't return an AxisArray anyway
julia> A
2-dimensional AxisArray{Int64,2,...} with axes:
:foo, ["a", "b"]
:bar, [:x, :y, :z]
And data, a 2×3 Matrix{Int64}:
1 2 3
4 5 6
julia> A .^ 3
2×3 Matrix{Int64}:
1 8 27
64 125 216
julia> A .+ 1
2×3 Matrix{Int64}:
2 3 4
5 6 7
another reason to drop it
|
||
end | ||
|
||
if ArrayType == KeyedArray |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forget, are there issues with AxisArray here? Or are you just not bothering?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dims syntax for AxisArrays expects an Axis
type not a Symbol
so it's incompatible with our interface unless we call axisdim(A, Axis{:foo})
or something to extract the dimension index.
Something we could do maybe to support this without depending on AxisArrays is:
if hasfield(A, axes)
dims = only(findall(a -> a == dims || typeof(a) <: dims, A.axes))
end
but I'd prefer to implement that at a later time if we actually need it (and I'd almost prefer to drop AxisArrays than do that).
0d2e038
to
00376b7
Compare
I'm gonna force merge this even though tests aren't passing - I'm not sure what's going on but they are passing over in #93 so they should at least be fixed with those. |
Note: I wrote the individual testsets for each data type before going around and removing them from each transform. In some cases this removed all the code so I left behind some unit tests specific to the transform.
In a follow up PR or commit I'd like to refactor the transform tests, but that is out of scope for this PR.