-
-
Notifications
You must be signed in to change notification settings - Fork 16
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
Fix doctest #1199
Fix doctest #1199
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1199 +/- ##
=======================================
Coverage 91.87% 91.87%
=======================================
Files 34 34
Lines 15366 15366
=======================================
Hits 14118 14118
Misses 1248 1248 ☔ View full report in Codecov by Sentry. |
@@ -65,7 +65,7 @@ julia> C.U | |||
⋅ ⋅ 3.0 | |||
|
|||
julia> C.L | |||
3×3 LowerTriangular{Float64, Adjoint{Matrix{Float64}}}: | |||
3×3 LowerTriangular{Float64, {Matrix{Float64}}: |
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.
This doesn't look right, or? Maybe
3×3 LowerTriangular{Float64, {Matrix{Float64}}: | |
3×3 LowerTriangular{Float64, Matrix{Float64}}: |
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.
There must be a mismatch between used code and tests in the failing doctest. This has been changed deliberately lately IIRC.
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.
My guess is that the docs don't run with the custom sysimage and are in fact testing the bundled LinearAlgebra. Could that be correct?
Can someone look at https://buildkite.com/julialang/julia-master/builds/44616#0194f191-ee57-4a81-ab55-0192700d1df9/823-1641? I don't see why Adjoint is in the output for LowerTriangular in the CI, both before and after:
None of this got caught in the CI in this repo - and at least in my local build on |
I still don't understand how these tests pass here but not on julia master. |
Ok - the issue in this repo seems to be that the |
I quickly ran the doctest in my REPL manually, and it does show the |
This PR is certainly incorrect. We should do the whole fix the sysimg doctest thing. But there is a separate point about whether the Adjoint is a good idea. I suppose fix the sysimg thing and this doctest so that we can bump LinearAlgebra on julia master once again. Afterwards, if we can decide if we want to revert #1186. |
I'm closing this. @KristofferC spotted the typo in the doctest (which was truly failing) and fixed it. |
Fix doctest seen in:
https://buildkite.com/julialang/julia-master/builds/44616#0194f191-ee57-4a81-ab55-0192700d1df9