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 creating matrices based on hash tables #3983

Open
wants to merge 26 commits into
base: devel
Choose a base branch
from

Conversation

lindsayad
Copy link
Member

This is only supported by the PETSc backend

@roystgnr
Copy link
Member

roystgnr commented Nov 4, 2024

Those look like real failures - somehow in a bunch of use cases we're now trying to zero a matrix before it's initialized?

@lindsayad lindsayad marked this pull request as draft November 4, 2024 18:14
@lindsayad
Copy link
Member Author

Yea, I'll get back to this particular PR soon

@lindsayad lindsayad changed the title Allow creating matrices based on hash tables [WIP] Allow creating matrices based on hash tables Nov 6, 2024
@lindsayad lindsayad force-pushed the hash-table-matrices branch from 17dc9e1 to 3b318be Compare November 6, 2024 20:30
@lindsayad lindsayad changed the title [WIP] Allow creating matrices based on hash tables Allow creating matrices based on hash tables Nov 21, 2024
@moosebuild
Copy link

moosebuild commented Nov 25, 2024

Job Coverage, step Generate coverage on b964294 wanted to post the following:

Coverage

d6fe9d #3983 b96429
Total Total +/- New
Rate 62.31% 62.31% +0.00% 83.33%
Hits 72713 72728 +15 60
Misses 43991 43999 +8 12

Diff coverage report

Full coverage report

Warnings

  • New new line coverage rate 83.33% is less than the suggested 90.0%

This comment will be updated on new commits.

@lindsayad lindsayad marked this pull request as ready for review November 25, 2024 18:05
@lindsayad
Copy link
Member Author

this is ready for review

include/numerics/sparse_matrix.h Outdated Show resolved Hide resolved
include/numerics/sparse_matrix.h Outdated Show resolved Hide resolved
src/numerics/petsc_matrix.C Outdated Show resolved Hide resolved
include/numerics/petsc_matrix.h Show resolved Hide resolved
src/systems/system.C Outdated Show resolved Hide resolved
@lindsayad lindsayad marked this pull request as draft December 18, 2024 06:20
@roystgnr
Copy link
Member

I take it there's no way via PETSc to get around the need for copy_from_hash()? I'd have thought the way to read hashed_matrix(i,j) would be to hash the pair (i,j) and read what you find (possibly after a close() in parallel), not to copy it into a CSR matrix or whatever and do all reads from that.

Other than that this is looking good now.

@lindsayad
Copy link
Member Author

The thing is that there is no "hash table" matrix in PETSc. There is just a "hash table until you call MatAssemblyBegin/End and then you have your CSR" matrix. So the moment you call close on the "temporarily hash table" matrix you create your CSR matrix and your nonzero pattern is set. So this copy_from_hash is necessary to get intermediate snapshots of your matrix before its nonzero pattern has been completely filled. These intermediate snapshots are needed for things like kinematic contact. Maybe reading over this PETSc issue would give some more context

@lindsayad
Copy link
Member Author

As the new example test is currently written, we will need https://gitlab.com/petsc/petsc/-/merge_requests/8063 to fix the failure. I'll probably just skip resetting the preallocation for the "non-hash-table" matrix in the mean time

@lindsayad
Copy link
Member Author

I have no idea how this broke MetaPhysicL

@lindsayad
Copy link
Member Author

Huzzah!

@lindsayad lindsayad marked this pull request as ready for review December 24, 2024 17:51
@lindsayad lindsayad requested a review from roystgnr December 24, 2024 17:51
Copy link
Member

@roystgnr roystgnr left a comment

Choose a reason for hiding this comment

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

This looks ready to go to me, but it's a big enough changeset that I'd say let's let @jwpeterson also have a chance to agree or object before merging.

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