-
Notifications
You must be signed in to change notification settings - Fork 55
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
Use mfem::Workspace
for temporary Vector
allocations
#194
base: main
Are you sure you want to change the base?
Conversation
f6ea580
to
2b7f73b
Compare
8cb0852
to
dcdd063
Compare
d70f023
to
6c7d418
Compare
dcdd063
to
6a95817
Compare
272a6b1
to
eaa4af8
Compare
74e35d9
to
83f60dd
Compare
eaa4af8
to
c7242ac
Compare
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.
Only important thing is the ComplexWorkspaceVector
I think has some very minor issues on the interface/usage of method casting. Otherwise, this is a great change.
palace/utils/workspace.hpp
Outdated
ComplexWorkspaceVector &operator=(ComplexWorkspaceVector &other) | ||
{ | ||
ComplexVector::operator=(other); | ||
return *this; | ||
} |
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 copy ctor and assignment I think should have const
on the arguments: the =delete
above doesn't refer to an existing special member function, and given it dispatches to the underlying copy ctor with a const argument, nothing should be lost. Looking at the mfem workspace, I see that's where the lack of const
is being copied from, so I asked Will a question there why he dropped it.
Additionally, looking in chebyshev.cpp
, I'm slightly confused by the corresponding need for .VecType::operator=(x)
which I would think of being for finding a shadowed method. However, I think the type system should be deducing this ok, as the overload resolution for operator=
should find the most specialized method. In the case the rhs is a ComplexVector
, it's operator=(const ComplexVector &)
if it's a ComplexWorkspaceVector
it would be operator=(const ComplexWorkspaceVector &)
. If that isn't working, do you have an idea why?
I was trying to trigger a dispatch error that would require this here but can't get the kind of dispatch error I think this must be addressing, hence the confusion.
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.
Good catch, indeed this error was copied from the original PR. Fixed in 8e4148a.
palace/linalg/rap.cpp
Outdated
auto ly = workspace::NewVector<Vector>(test_fespace.GetVSize()); | ||
A->Mult(lx, ly); | ||
|
||
auto &ty = test_fespace.GetTVector<Vector>(); | ||
RestrictionMatrixMult(ly, ty); | ||
b.Add(-1.0, ty); | ||
{ |
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.
ly
can be moved into the limited scope too.
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.
This scope block is actually not necessary I think since ly
and ty
must always be present in the same scope. So I've removed in f3ef607 to be more similar to the rest of the ParOperator
functions.
6b971ca
to
4962970
Compare
98ac8f7
to
f01618c
Compare
705e2ba
to
19f9618
Compare
f01618c
to
73298c8
Compare
19f9618
to
36b3cfb
Compare
73298c8
to
ba69315
Compare
702eb30
to
ffaeafd
Compare
ba69315
to
3e5a42b
Compare
Commit 3e5a42b introduces a patch for MFEM to fix the issues discussed here: mfem/mfem#4065 (comment). |
1e79930
to
9072819
Compare
9072819
to
c3b4ac4
Compare
NOTE: Builds on #184 (and #194, and now #204)
See mfem/mfem#4065
TODO: Add patch on MFEM for mfem/mfem#4065 to enable this functionality-> Done as part of #184