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

BlockOperator and BlockDiagonalOperator #215

Merged
merged 11 commits into from Feb 19, 2016

Conversation

Projects
None yet
3 participants
@pmli
Member

pmli commented Feb 2, 2016

Code: src/pymor/playground/block.py
Tests: src/pymortests/block.py

@sdrave sdrave referenced this pull request Feb 13, 2016

Closed

[blocks] #79

self.num_source_blocks = len(source_types)
self.num_range_blocks = len(range_types)
self.linear = all(op.linear for op in self._operators())
self._is_diagonal = (all(block is None if i != j else True for (i, j), block in np.ndenumerate(self._blocks))

This comment has been minimized.

@ftalbrecht

ftalbrecht Feb 15, 2016

Member

is_diagonal is not used

This comment has been minimized.

@pmli

pmli Feb 15, 2016

Member

Agreed. Should it be used somehow or can it be removed?

This comment has been minimized.

@ftalbrecht

ftalbrecht Feb 15, 2016

Member

I guess I would just remove it...

block = V.copy()
else:
block += V
blocks.append(block.copy())

This comment has been minimized.

@ftalbrecht

This comment has been minimized.

@pmli

pmli Feb 15, 2016

Member

s.a. = ? 😕

This comment has been minimized.

@ftalbrecht

ftalbrecht Feb 15, 2016

Member

Ah, sorry for that. I meant s.a. as in see above. As in:

  • if we do not need copy() above, then we do not need it here, either
  • if you agree that a pre-initialization would make sense above, then I would also pre-initialize blocks as a list of appropriate length (num_source_blocks) here. The same holds further below...
def apply_inverse(self, V, ind=None, mu=None, least_squares=False):
U = []
for i in xrange(self.num_source_blocks):
U.append(self._blocks[i, i].apply_inverse(V.block(i), ind=ind, mu=mu, least_squares=least_squares))

This comment has been minimized.

@ftalbrecht
@ftalbrecht

This comment has been minimized.

Member

ftalbrecht commented Feb 15, 2016

Sorry for being so late. I left a few notes, please take a look. Apart from that: looks good to me!

@sdrave sdrave added this to the 0.4 milestone Feb 18, 2016

@sdrave

This comment has been minimized.

Member

sdrave commented Feb 18, 2016

Looks good to me! When merging I will only update the copyright notice and remove the import from the __init__.py (we have the policy of not importing subpackages). I believe we could also directly move this from the playground to pymor.operators.block?

@ftalbrecht

This comment has been minimized.

Member

ftalbrecht commented Feb 18, 2016

@sdrave sdrave merged commit 20c61fa into pymor:master Feb 19, 2016

1 of 2 checks passed

code-quality/landscape Landscape is checking code quality
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sdrave

This comment has been minimized.

Member

sdrave commented Feb 19, 2016

Hi @pmli, thank you very much for your contribution! Your code is now in master and I have added an attribution to AUTHORS.md.

By adding fixtures for BlockOperator and BlockDiagonalOperator to our testing framework, I discovered a small bug in your BlockDiagonalOperator.apply_inverse_adjoint implementation in the case when scalar products are present: The inverse adjoint operator maps from source to range, so the source_product needs to be applied first, not the range_product. I simplified the code by calling the default implementation which takes care of the scalar products, so only the no-product case has to be implemented.

@pmli pmli deleted the pmli:block-operator2 branch Mar 2, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment