Skip to content

Add tests with JET #955

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 4, 2025
Merged

Add tests with JET #955

merged 1 commit into from
May 4, 2025

Conversation

@devmotion
Copy link
Member

@devmotion devmotion commented Apr 29, 2025 •
edited
Loading

Based on and opened against #954.

The most basic JET tests revealed a typo and few issues (calls of non-existing methods):

julia> using StatsBase, JET

julia> report_package(StatsBase; target_defined_modules = true)
...
═════ 6 possible errors found ═════
┌ _unscaled_covzm(x::DenseMatrix, dims::Colon) @ StatsBase /Users/david/.julia/packages/StatsBase/uvhDX/src/cov.jl:26
│ no matching method found `unscaled_covzm(::DenseMatrix)`: StatsBase.unscaled_covzm(x::DenseMatrix)
└────────────────────
┌ _unscaled_covzm(x::DenseMatrix, wv::StatsBase.AbstractWeights, dims::Colon) @ StatsBase /Users/david/.julia/packages/StatsBase/uvhDX/src/cov.jl:29
│ no matching method found `_scalevars(::DenseMatrix, ::StatsBase.AbstractWeights)`: StatsBase._scalevars(x::DenseMatrix, wv::StatsBase.AbstractWeights)
└────────────────────
┌ (::Base.TTY)(obj::StatisticalModel) @ StatsBase /Users/david/.julia/packages/StatsBase/uvhDX/src/deprecates.jl:8
│ `StatsBase.MethodErrror` is not defined: StatsBase.MethodErrror
└────────────────────
┌ model_response(obj::StatisticalModel) @ StatsBase ./deprecated.jl:105
│ no matching method found `response(::StatisticalModel)`: response(obj::StatisticalModel)
└────────────────────
┌ scattermat_zm(x::DenseMatrix, dims::Int64) @ StatsBase ./deprecated.jl:105
│ no matching method found `kwcall(::@NamedTuple{dims::Int64}, ::typeof(StatsBase.scattermat_zm), ::DenseMatrix)`: Core.kwcall(NamedTuple{(:dims,)}(tuple(dims::Int64)::Tuple{Int64})::@NamedTuple{dims::Int64}, scattermat_zm, x::DenseMatrix)
└────────────────────
┌ scattermat_zm(x::DenseMatrix, wv::StatsBase.AbstractWeights, dims::Int64) @ StatsBase ./deprecated.jl:105
│ no matching method found `kwcall(::@NamedTuple{dims::Int64}, ::typeof(StatsBase.scattermat_zm), ::DenseMatrix, ::StatsBase.AbstractWeights)`: Core.kwcall(NamedTuple{(:dims,)}(tuple(dims::Int64)::Tuple{Int64})::@NamedTuple{dims::Int64}, scattermat_zm, typeassert(x::DenseMatrix, StatsBase.DenseMatrix)::DenseMatrix, typeassert(wv::StatsBase.AbstractWeights, AbstractWeights)::StatsBase.AbstractWeights)
└────────────────────

Edit: Tests currently fail on 1.12 (only somewhat experimental support for JET, an internal AssertionError is thrown) and on the other Julia versions since JET triggers method redefinitions (e.g. https://github.com/JuliaStats/StatsBase.jl/actions/runs/14743207189/job/41385529150?pr=955#step:6:178) that cause other subsequent tests to fail. See https://aviatesk.github.io/JET.jl/v0.9/jetanalysis/#jetanalysis-toplevel-entry

Edit 2: "Fixed" by disabling JET tests on Julia 1.12 and moving the JET tests to the end, after all other tests.

Base automatically changed from dw/julia_compat to master April 30, 2025 07:40
@devmotion devmotion force-pushed the dw/jet_tests branch 2 times, most recently from 0f0ec5a to 546cddb Compare April 30, 2025 08:02
devmotion
devmotion commented Apr 30, 2025
View reviewed changes
src/cov.jl

## scatter matrix

_unscaled_covzm(x::DenseMatrix, dims::Colon) = unscaled_covzm(x)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There exists no unscaled_covzm with this type signature in Statistics.

devmotion
devmotion commented Apr 30, 2025
View reviewed changes
src/cov.jl
_unscaled_covzm(x::DenseMatrix, dims::Colon) = unscaled_covzm(x)
_unscaled_covzm(x::DenseMatrix, dims::Integer) = unscaled_covzm(x, dims)

_unscaled_covzm(x::DenseMatrix, wv::AbstractWeights, dims::Colon) =
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here.

devmotion
devmotion commented Apr 30, 2025
View reviewed changes
src/deprecates.jl
else
function (io::typeof(stderr))(obj::StatisticalModel)
Base.depwarn("stderr(obj::StatisticalModel) is deprecated, use stderror(obj) instead", :stderr)
io === stderr ? stderror(obj) : throw(MethodErrror(io, (obj,)))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A typo.

devmotion
devmotion commented Apr 30, 2025
View reviewed changes
src/deprecates.jl
Comment on lines -25 to -26
@deprecate scattermat_zm(x::DenseMatrix, dims::Int) scattermat_zm(x, dims=dims)
@deprecate scattermat_zm(x::DenseMatrix, wv::AbstractWeights, dims::Int) scattermat_zm(x::DenseMatrix, wv::AbstractWeights, dims=dims)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also these do not exist.

devmotion
devmotion commented Apr 30, 2025
View reviewed changes
@devmotion devmotion requested a review from andreasnoack April 30, 2025 08:12
@devmotion devmotion mentioned this pull request May 3, 2025
Merged
@andreasnoack andreasnoack merged commit dfe8945 into master May 4, 2025
12 of 13 checks passed
@andreasnoack andreasnoack deleted the dw/jet_tests branch May 4, 2025 19:37
@nalimilan
Copy link
Member

JET makes tests fail due to version compat issues at #959. Any ideas what may be going on?

@devmotion
Copy link
Member Author

It seems to be a bug in Pkg: #959 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@andreasnoack andreasnoack andreasnoack approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@devmotion @nalimilan @andreasnoack