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

Draft
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 f102d7c wanted to post the following:

Coverage

3fbbdd #3983 f102d7
Total Total +/- New
Rate 62.31% 62.31% +0.00% 81.69%
Hits 72713 72727 +14 58
Misses 43991 43999 +8 13

Diff coverage report

Full coverage report

Warnings

  • New new line coverage rate 81.69% 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
pr.second->use_hash_table(use_hash);
if (!use_hash)
this->_require_sparsity_pattern = true;
}
Copy link
Member

Choose a reason for hiding this comment

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

It looks like we definitely need some test coverage of a system with a mix of hash and non-hash matrices. Even if it's not hit in CI until we upgrade PETSc there, we'll at least get some early warning on our own machines.

Copy link
Member Author

Choose a reason for hiding this comment

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

We already have a PETSc sufficient for testing in apptainer. I will look into making something

Copy link
Member Author

Choose a reason for hiding this comment

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

I still need to add some testing for this

Copy link
Member Author

Choose a reason for hiding this comment

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

k done

@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

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