Skip to content
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

Run CI on latest released 1.x version #928

Merged
merged 4 commits into from Jul 1, 2021
Merged

Conversation

thchr
Copy link
Collaborator

@thchr thchr commented Jun 19, 2021

I noticed that CI wasn't running on 1.6, which I figured would be a good idea, even if it increases the run-time.

With this PR, it still doesn't run on 1.5, and won't run on 1.6 after 1.7 is released (then it'll run on 1.7) - but it seems better than not to run at all, and doesn't increase CI time very much compared to adding everything.

@thchr
Copy link
Collaborator Author

thchr commented Jun 20, 2021

I added some fixes to tests, so that this should now ideally also pass the test-suite on v1.6.

All test failures turned out to have been noted in the issue tracker already.

Closes #912, #924, #925

@thchr
Copy link
Collaborator Author

thchr commented Jun 20, 2021

Huh, seems this didn't quite fix #925. Not sure if this isn't really a bug in Julia instead. Specifically, if we define:

julia> using StaticArrays, Test, LinearAlgebra
julia> m_sing2 = @SMatrix [1 1; 1 0; 0 1];
julia> svd_full_false(A) = svd(A, full=false);

and consult @code_warntype, everything is fine:

julia> @code_warntype svd_full_false(m_sing2)
Variables
  #self#::Core.Const(svd_full_false)
  A::SMatrix{3, 2, Int64, 6}

Body::StaticArrays.SVD{Float64, SMatrix{3, 2, Float64, 6}, SVector{2, Float64}, SMatrix{2, 2, Float64, 4}}
1%1 = (:full,)::Core.Const((:full,))
│   %2 = Core.apply_type(Core.NamedTuple, %1)::Core.Const(NamedTuple{(:full,), T} where T<:Tuple)
│   %3 = Core.tuple(false)::Core.Const((false,))
│   %4 = (%2)(%3)::Core.Const((full = false,))
│   %5 = Core.kwfunc(Main.svd)::Core.Const(LinearAlgebra.var"#svd##kw"())
│   %6 = (%5)(%4, Main.svd, A)::StaticArrays.SVD{Float64, SMatrix{3, 2, Float64, 6}, SVector{2, Float64}, SMatrix{2, 2, Float64, 4}}
└──      return %6

But @inferred insists this isn't inferred (contrary to @code_warntype):

julia> @inferred svd_full_false(m_sing2)
ERROR: return type StaticArrays.SVD{Float64, SMatrix{3, 2, Float64, 6}, SVector{2, Float64}, SMatrix{2, 2, Float64, 4}} does not match inferred return type Union{StaticArrays.SVD{Float64, SMatrix{3, 2, Float64, 6}, SVector{2, Float64}, SMatrix{2, 2, Float64, 4}}, StaticArrays.SVD{Float64, SMatrix{3, 3, Float64, 9}, SVector{2, Float64}, SMatrix{2, 2, Float64, 4}}}
Stacktrace:
 [1] error(s::String)
   @ Base ./error.jl:33
 [2] top-level scope
   @ REPL[19]:1

(Edit: this difference between @code_warntype and @inferred might just be JuliaLang/julia#32834)

Pretty sure I even had one REPL session where @inferred seemed to be working, but then it didn't in another session.

@thchr
Copy link
Collaborator Author

thchr commented Jun 20, 2021

Okay; I just disabled [edit: marked as broken] the svd(A, full=false) kwarg constant-propagation test on v1.6 and later: for whatever reason, it just doesn't seem to work (even though, puzzlingly, small toy-examples with constant propagation of keyword arguments certainly do work).

…d `@test_broken` for `abstractarray.jl` tests
@thchr
Copy link
Collaborator Author

thchr commented Jun 24, 2021

This now also changes a few @test_broken in test/abstractarrays.lj to @test on the nightly version of Julia (they got fixed by JuliaLang/julia#40831 apparently).

That's basically unrelated to this PR - but I just wanted to see if I could get the nightly CI passing (sadly, there were more issues down the line - I'm going to leave them for now).

@KristofferC
Copy link
Contributor

Great, thanks. @mateuszbaran can this be merged? It would be nice to have StaticArrays test pass on latest release or otherwise regressions won't be noticed by PkgEval for upcoming releases (e.g. 1.7).

@mateuszbaran
Copy link
Collaborator

Yes, right, I'll merge this. I think this doesn't require a new release because only tests are changed.

@KristofferC
Copy link
Contributor

True but unless it gets a release PkgEval won't pick it up. However, there might be other things that will require a release soon anyway, like the reshape thing.

@mateuszbaran
Copy link
Collaborator

Yes, I think both that reshape thing and #931 require urgent fixing. I'm a bit busy today but I should be able work on this tomorrow if no one else does.

@mateuszbaran mateuszbaran merged commit 1a06987 into JuliaArrays:master Jul 1, 2021
@thchr
Copy link
Collaborator Author

thchr commented Jul 1, 2021

#932 should fix the qr ambiguity problem (as well as the lu ambiguities that remained on nightly testing on this PR).

thchr added a commit to thchr/StaticArrays.jl that referenced this pull request Jul 8, 2021
* Run CI on latest released 1.x version

* fix broken tests on v1.6+

* disable `svd` kwarg const-prop test on v1.6 and later

* mark `svd` test as broken on v1.6 and later & swap between `@test` and `@test_broken` for `abstractarray.jl` tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants