Skip to content

Conversation

@rikhuijzer
Copy link

@rikhuijzer rikhuijzer commented Mar 5, 2022

I've tested these changes on Julia 1.6, 1.7 and 1.8-beta1.

I've also changed

on:
  - push
  - pull_request

to

on:
  push:
    branches:
      - master
  pull_request:

to avoid triggering the jobs for pushes and pull requests at the same time. See, for example, https://github.com/JuliaData/CSV.jl/blob/2c02394b4ce11ee673ed1b6d0c811a98316bc2f9/.github/workflows/ci.yml#L3-L5

@mbauman mbauman closed this Mar 6, 2022
@mbauman mbauman reopened this Mar 6, 2022
Rik Huijzer and others added 4 commits March 6, 2022 15:02
@rikhuijzer
Copy link
Author

Can someone approve the workflow?

@codecov
Copy link

codecov bot commented Mar 6, 2022

Codecov Report

Merging #205 (a6f6d9d) into master (de595e0) will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #205      +/-   ##
==========================================
+ Coverage   93.79%   93.81%   +0.02%     
==========================================
  Files          13       13              
  Lines        2208     2265      +57     
==========================================
+ Hits         2071     2125      +54     
- Misses        137      140       +3     
Impacted Files Coverage Δ
src/LazyArrays.jl 100.00% <ø> (ø)
src/lazybroadcasting.jl 96.20% <0.00%> (-0.90%) ⬇️
src/lazyapplying.jl 84.89% <0.00%> (-0.37%) ⬇️
src/cache.jl 97.21% <0.00%> (+0.02%) ⬆️
src/linalg/mul.jl 84.12% <0.00%> (+0.08%) ⬆️
src/lazyoperations.jl 96.71% <0.00%> (+0.19%) ⬆️
src/lazyconcat.jl 95.79% <0.00%> (+0.20%) ⬆️
src/linalg/add.jl 86.27% <0.00%> (+0.27%) ⬆️
src/lazysetoperations.jl 85.71% <0.00%> (+1.50%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update de595e0...a6f6d9d. Read the comment docs.

@dlfivefifty
Copy link
Member

The fixes look like they are working around regressions so I don't think this should be merged as is. E.g. replacing with == in

@test apply(*, Eye(10), Ones(10))  Ones(10)

is an issue: we want it to be identically a Ones, not just have the same entries.

@rikhuijzer
Copy link
Author

rikhuijzer commented Mar 6, 2022

is an issue: we want it to be identically a Ones, not just have the same entries.

Do you have a suggestion on how to fix it? I think that the failures are due to the new representations for sparse matrices

EDIT: Specifically, JuliaLang/julia#42577

@dlfivefifty
Copy link
Member

Just need to step through the debugger and see what's going wrong

@test a .* v == v .* a

@test n .+ n Vcat(2,Zeros{Int}(3))
@test n .+ v n .+ v
Copy link
Author

@rikhuijzer rikhuijzer Mar 7, 2022

Choose a reason for hiding this comment

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

I've added this one because currently this doesn't even hold on Julia 1.8-beta1 (due to JuliaArrays/FillArrays.jl#172). Testing this may save a lot of time in the future when trying to figure out why n .+ v !== v .+ n.

@rikhuijzer
Copy link
Author

rikhuijzer commented Mar 7, 2022

Tests are still broken. Patch for FillArrays is at JuliaArrays/FillArrays.jl#172.

@dlfivefifty dlfivefifty merged commit db3ea4e into JuliaArrays:master Mar 20, 2022
@rikhuijzer
Copy link
Author

Awesome! Thanks for fixing this and MatrixFactorizations, @dlfivefifty

@rikhuijzer rikhuijzer deleted the rh/fix-1.8 branch March 20, 2022 09:01
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.

5 participants