-
Notifications
You must be signed in to change notification settings - Fork 148
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 #963: PETSc 3.20.0 broke the world #982
Conversation
The two were confusingly redundant: - PySparse takes `sizeHint`, the number of non-zeros in the matrix. - PyTrilinos takes `NumEntriesPerRow`. - petsc4py didn't used to be clear what it took, but is now documented as number of non-zeros per row (of the local portion of the matrix, but we'll ignore that part). - scipy doesn't preallocate. - Linear algebra [defines bandwidth](https://en.wikipedia.org/wiki/Band_matrix#Bandwidth) as "number $k$ such that $a_{i,j}=0$ if $|i-j| > k$", which is roughly half the number of non-zeros per row (and only applies to a band-diagonal matrix). Better to be explicit about what we really mean. Now all take same parameter and PySparse adjusts as needed. `sizeHint` was introduced in @a15d696 (in 2006!) to "allow smaller preallocations", but it was never used that way. Now, `nonZerosPerRow` can take an array_like to specify row-by-row preallocations, which are directly supported by PyTrilinos and petsc4py, and can be simulated for PySparse. Added `exactNonZeros`, which may have performance benefits for PyTrilinos and petsc4py. Currently unused.
Use Term's knowledge of its stencil to preallocate its matrix more effectively.
The one failure is due to an unrelated intermittent race condition |
Now, the one failure is due to cosmic rays ☢️ 😠 |
@@ -258,7 +258,7 @@ stages: | |||
condition: startsWith(variables.image, 'windows') | |||
- bash: | | |||
mamba create --quiet --name wheelEnvironment --channel conda-forge python=3.10 mamba | |||
conda create --quiet --name wheelEnvironment --channel conda-forge python=3.10 |
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.
Why back to conda?
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.
Because conda now uses libmamba (or at least can be configured to do so, which I do)
if matrix is None: | ||
tmpMatrix = spmatrix.ll_mat(1, 1, 1) | ||
try: |
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.
Can you do this without a try / except? It just feels wrong here. Does the asarray
call raise the TypeError
?
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.
The len(nonZerosPerRow)
is what throws the TypeError
. This is the most effective way to figure out if nonZerosPerRow
is iterable in Python 2.7. In Py3k, you can (and I do) do an isinstance(nonZerosPerRow, collections.abc.Iterable)
.
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.
Oh yeah, FiPy still needs to work with 2.7.
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.
Hopefully not for much longer. My plan is to finish the solver harmonization I'm working on, make a release with everything, and then immediately drop PySparse and Python 2.7
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.
Did not test myself, but the changes look good to me.
Fixes #963 (Actually, PETSC 3.19.0 broke the world.) This PR: - Assembles before `multTranspose` to prevent newly added exception - Renames `bandwidth` to `nonZerosPerRow` and removes `sizeHint` The two were confusingly redundant: - PySparse takes `sizeHint`, the number of non-zeros in the matrix. - PyTrilinos takes `NumEntriesPerRow`. - petsc4py didn't used to be clear what it took, but is now documented as number of non-zeros per row (of the local portion of the matrix, but we'll ignore that part). - scipy doesn't preallocate. - Linear algebra [defines bandwidth](https://en.wikipedia.org/wiki/Band_matrix#Bandwidth) as "number $k$ such that $a_{i,j}=0$ if $|i-j| > k$", which is roughly half the number of non-zeros per row (and only applies to a band-diagonal matrix). Better to be explicit about what we really mean. Now all take same parameter and PySparse adjusts as needed. `sizeHint` was introduced in @a15d696 (in 2006!) to "allow smaller preallocations", but it was never used that way. Now, `nonZerosPerRow` can take an array_like to specify row-by-row preallocations, which are directly supported by PyTrilinos and petsc4py, and can be simulated for PySparse. Added `exactNonZeros`, which may have performance benefits for PyTrilinos and petsc4py. Currently unused. - Uses `Term`'s knowledge of own stencil to preallocate more effectively. Still doesn't do a good job with vector equations, but that's a deeper change (the resolution of which might help #920). - Fixes(?) conda/mamba installs - Fixes(?) race condition
Fixes #963
(Actually, PETSC 3.19.0 broke the world.)
This PR:
Assembles before
multTranspose
to prevent newly added exceptionRenames
bandwidth
tononZerosPerRow
and removessizeHint
The two were confusingly redundant:
sizeHint
, the number of non-zeros in the matrix.NumEntriesPerRow
.documented as number of non-zeros per row (of the local portion
of the matrix, but we'll ignore that part).
defines bandwidth
as "number
roughly half the number of non-zeros per row (and only applies
to a band-diagonal matrix).
Better to be explicit about what we really mean.
Now all take same parameter and PySparse adjusts as needed.
sizeHint
was introduced in @a15d696 (in 2006!) to"allow smaller preallocations", but it was never used that way.
Now,
nonZerosPerRow
can take an array_like to specify row-by-rowpreallocations, which are directly supported by PyTrilinos and petsc4py,
and can be simulated for PySparse.
Added
exactNonZeros
, which may have performance benefits forPyTrilinos and petsc4py. Currently unused.
Uses
Term
's knowledge of own stencil to preallocate more effectively.Still doesn't do a good job with vector equations, but that's a deeper change
(the resolution of which might help Should be possible to couple scalar and vector equations #920).