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

Allow Int32 indices, switch to SuiteSparse 7 #380

Merged
merged 16 commits into from
Jul 9, 2023
Merged

Allow Int32 indices, switch to SuiteSparse 7 #380

merged 16 commits into from
Jul 9, 2023

Conversation

rayegun
Copy link
Member

@rayegun rayegun commented Apr 4, 2023

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Apr 4, 2023

Codecov Report

Merging #380 (e953cf0) into main (6ccc974) will decrease coverage by 0.40%.
The diff coverage is 80.41%.

❗ Current head e953cf0 differs from pull request most recent head cc518fa. Consider uploading reports for the commit cc518fa to get more accurate results

@@            Coverage Diff             @@
##             main     #380      +/-   ##
==========================================
- Coverage   93.97%   93.57%   -0.40%     
==========================================
  Files          12       12              
  Lines        7480     7520      +40     
==========================================
+ Hits         7029     7037       +8     
- Misses        451      483      +32     
Impacted Files Coverage Δ
src/linalg.jl 96.09% <ø> (ø)
src/solvers/cholmod.jl 87.50% <80.14%> (-3.16%) ⬇️
src/solvers/spqr.jl 86.41% <88.88%> (ø)

... and 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@rayegun
Copy link
Member Author

rayegun commented Apr 4, 2023

I'm going to have to test this on a 32-bit VM to get anywhere on this

@ViralBShah
Copy link
Member

In case you don't already have one, the easiest thing is to get the ubuntu 32 bit docker image on a linux box.

@ViralBShah
Copy link
Member

ViralBShah commented Apr 12, 2023

@rayegun
Copy link
Member Author

rayegun commented Apr 12, 2023

Okay that's good info, I'm not surprised QR is a bit worse, I made much more cursory changes there. I'll update.

Will do doctests too.

@rayegun
Copy link
Member Author

rayegun commented Apr 12, 2023

The newest version of SPQR will introduce a small regression for us, we have to convert HPinv from Int32->Int64->Int32 when Ti === Int64. There are no _l_ vs _ methods for int32_t in SPQR as far as I can tell.

@ViralBShah
Copy link
Member

Is this breaking for the Julia user, or is it just that we need to adjust SPQR bindings to do some extra conversions so that tests pass?

@rayegun
Copy link
Member Author

rayegun commented Apr 13, 2023

Just extra conversions.

@rayegun
Copy link
Member Author

rayegun commented Apr 17, 2023

@ViralBShah to unblock progress I've temporarily taken out the SPQR 32-bit plat tests. These should "just work" after the upgrade to SS7.1 when Tim fixes this upstream.

I don't see another option rn

@ViralBShah
Copy link
Member

I suppose we should wait for SuiteSparse 7.1.

test/spqr.jl Outdated Show resolved Hide resolved
@rayegun rayegun merged commit 2e15e8b into main Jul 9, 2023
1 of 6 checks passed
@rayegun rayegun deleted the rk/ss7 branch July 9, 2023 01:02
@ViralBShah ViralBShah changed the title Allow Int32 indices, switch to SS7 Allow Int32 indices, switch to SuiteSparse 7 Jul 9, 2023
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.

3 participants