-
Notifications
You must be signed in to change notification settings - Fork 177
Assembly of matfree interpolation operator #4778
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
base: main
Are you sure you want to change the base?
Conversation
57e5d2d to
5019974
Compare
| submat = o.petscmat.createSubMatrix(*ises) | ||
| bcs = () | ||
| return AssembledMatrix(tuple(args), bcs, submat) | ||
| return AssembledMatrix(tuple(args), submat) |
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.
What happened to the BCs?
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.
BTW, this will be fixed in https://github.com/firedrakeproject/firedrake/pull/4708/files
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.
BCs are an optional argument now
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.
Better to not change this line to avoid conflicts
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.
Conflicts with what?
update comment
75de8e2 to
5464cce
Compare
connorjward
left a comment
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.
I'm only reviewing this from a high level, but this seems pretty much fine.
| def __init__( | ||
| self, | ||
| a: ufl.BaseForm | TensorBase | tuple[BaseArgument, BaseArgument], | ||
| bcs: Iterable[BCBase] | None = None, |
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 is this allowed to be None? The default could just be ().
| return "%s(a=%r, bcs=%r)" % (type(self).__name__, | ||
| self.a, | ||
| self.bcs) | ||
| return f"{type(self).__name__}(a={repr(self.a)}, bcs={repr(self.bcs)})" |
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.
| return f"{type(self).__name__}(a={repr(self.a)}, bcs={repr(self.bcs)})" | |
| return f"{type(self).__name__}(a={self.a!r}, bcs={self.bcs!r})" |
| def __str__(self): | ||
| return "assembled %s(a=%s, bcs=%s)" % (type(self).__name__, | ||
| self.a, self.bcs) | ||
| return f"assembled {type(self).__name__}(a={str(self.a)}, bcs={str(self.bcs)})" |
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.
| return f"assembled {type(self).__name__}(a={str(self.a)}, bcs={str(self.bcs)})" | |
| return f"assembled {type(self).__name__}(a={self.a}, bcs={self.bcs})" |
| bcs: Iterable[BCBase] | None = None, | ||
| options_prefix: str | None = None, | ||
| ): | ||
| """A representation of a matrix that doesn't require knowing the underlying form. |
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.
Just to note, I think that it's safe to have this docstring below the class name instead of inside __init__. Not too bothered either way though.
| submat = o.petscmat.createSubMatrix(*ises) | ||
| bcs = () | ||
| return AssembledMatrix(tuple(args), bcs, submat) | ||
| return AssembledMatrix(tuple(args), submat) |
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.
Conflicts with what?
|
@connorjward Thanks for the review. I'm at a workshop this week and then on holiday again next week so I'll take a look after these. Thanks! |
Allow assembly of matfree interpolation operators by passing
mat_type="matfree".Also changes the interfaces to the matrix classes and
ImplicitMatrixContextto make them simpler.