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

Implementation and test for matfree zeroRowsColumns #4

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ioannisPApapadopoulos
Copy link
Owner

@ioannisPApapadopoulos ioannisPApapadopoulos commented Jul 16, 2020

This is an implementation for a matfree zeroRowsColumns method. Since we are dealing with unassembled matrices, we cannot perform the algebraic operation of zeroing the row and column. So instead when petsc4py passes along which row/column, needs to be zeroed, we find its associated dof and make a ZeroRowsColumnsBC object (which overloads the DirichletBC class) to impose the same effect as zeroing the rows and column of that dof and adding a 1 onto the diagonal. Tests show that this behaves as expected.

@@ -9,7 +9,6 @@
from firedrake.petsc import PETSc
from firedrake.utils import cached_property


Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't change things if you don't have to.

@@ -57,6 +56,88 @@ def find_sub_block(iset, ises):
return found


def find_element_of_which_sub_block(rows, ises):
# This function acts as lookup to find which block the indices belong in
Copy link
Collaborator

Choose a reason for hiding this comment

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

Needs more explanation in the comment. Try to convey the bigger picture.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Write what it takes in, what it returns, and why.

for i in range(step):
rows_i = numpy.extract(belong_to_sub == i, rows)
rows_i = rows_i - i
rows_i = numpy.int32(rows_i/step)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this safe if you have more than 4 billion dofs? Strange to hardcode int32.

@@ -106,6 +187,8 @@ def __init__(self, a, row_bcs=[], col_bcs=[],
self._y = function.Function(test_space)
self._x = function.Function(trial_space)

# Temporary storage for holding the BC values during zeroRowsColumns
self._tmp_zeroRowsColumns = function.Function(test_space)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Functions are, on average, expensive. (Imagine you have a problem with a billion dofs.) Can we make this so that this is only allocated if needed?

if active_rows.size == 0:
# This can happen when running in parallel. Every processor sharing the matrix
# must call zeroRowsColumns but the processor might not have any rows to zero.
# For now we just return without adding additional DirichletBC, is this the correct thing to do?
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that bc application is collective -- you should make a DirichletBC object on every processor, even if it's empty.

# must call zeroRowsColumns but the processor might not have any rows to zero.
# For now we just return without adding additional DirichletBC, is this the correct thing to do?
return
if not numpy.allclose(diag, 1.0):
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's wrong with if diag != 1?

if test.function_space().shape == ():
rhs_form = inner(Constant(1), test)*dx
elif test.function_space().shape == (2, ):
rhs_form = inner(Constant((1, 1)), test)*dx
Copy link
Collaborator

Choose a reason for hiding this comment

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

else: raise NotImplementedError

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.

2 participants