Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Fieldsplit: replace empty Forms with ZeroBaseForm #3947
base: master
Are you sure you want to change the base?
Fieldsplit: replace empty Forms with ZeroBaseForm #3947
Changes from 12 commits
2286596
bb04bb0
d82039d
af53302
7f40504
3d06fc5
6078f93
5894b49
d99ba50
d6bb7dd
ed58467
2a0c03b
934ff6f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 use the
subspace
function here too? It will need a switch adding for primal/dual but I think it would be nicer to usesubspace
for both.I think it would also be nice to lift
subspace
out, possibly into one of the functionspace files. It's useful more generally - see for example line 678 inslate.py
where we have the same if/else block to construct the subspace.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 was thinking to extend
FunctionSpace.__getitem__
to allow a list index return the subspace, since this is how sub-arrays are extracted in numpy.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 if statement can be avoided by using
FunctionSpace.collapse()
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 was actually thinking exactly the same thing the other day.
It would be great to have it for
Function
too so that you can get aMixed
subfunction viewing the relevant subcomponents.ExtractSubBlock.cofunction
would then basically just be callingcofunction[idxs]
.There are definitely other places where that would be useful too, for example in the
EnsembleFunction
I've been working on here.What does
FunctionSpace.collapse()
do? It just looks like it would return a copy of theFunctionSpace
.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.
@ksagiyam what do you think about this
__getitem__
idea? Currenty, indexing by a scalar returnsIndexedProxyFunctionSpace
, but indexing by a list of a single item would return the collapsed version of this space.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.
__getitem__
forFunction
is implemented in UFL so the changes would be needed there I think.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 not too worried for the
Function
case, that one seems more involved, as they are Expressions in the UFL sense, and extracting a component naturally means a scalar compoment, but here it seems that we want a MixedFunctionSpace (possibly vector-valued) component.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.
It could also be added to
sub
orsubfunctions
, although they both also have issues.sub
explicitly says it is for bcs, andsubfunctions
is already a tuple not a method so it would probably have to become a local class with__getitem__
overridden so we could deal with single or multiple indices.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.
That sounds slightly odd to me. Can you instead rewrite
collapse()
so that it would take a list of indices as an optional argument?@JHopeCollins
collapse()
basically forgets parents. They do something nontrivial in ProxyFunctionSpace and in MixedFunctionSpace.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.
Ok thanks.
It does strip the parent information, I don't think the name
collapse
would be where I would look for this functionality - it would be more intuitive in subfunctions or sub.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.
Is this to do in this PR or at a later date? If it won't be part of this PR please can you open an issue for it to keep a record?