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

Move all backend-agnostic assemble_lincomb logic to algorithms.lincomb #619

Merged
merged 16 commits into from
Mar 15, 2019

Conversation

sdrave
Copy link
Member

@sdrave sdrave commented Mar 8, 2019

This moves all assemble_lincomb methods for generic Operators into pymor.algorithms.lincomb. The RuleTable also takes care of eliminating ZeroOperators. ATM, the backends still need to handle IdentitiyOperators. Before merging, this issue should be resolved (e.g. by introducing a ShiftedOperator and apply_inverse_shifted interface methods or a separate argument to _assemble_lincomb).

This addresses #548.

@sdrave sdrave added pr:change Change in existing functionality interfaces labels Mar 8, 2019
@sdrave sdrave added this to the 2019.2 milestone Mar 8, 2019
@sdrave sdrave requested a review from pmli March 8, 2019 14:58
@sdrave sdrave added the pr:new-feature Introduces a new feature label Mar 14, 2019
@sdrave sdrave changed the title [WIP] Move all backend-agnostic assemble_lincomb logic to algorithms.lincomb Move all backend-agnostic assemble_lincomb logic to algorithms.lincomb Mar 14, 2019
@sdrave
Copy link
Member Author

sdrave commented Mar 14, 2019

@pmli I am now quite satisfied with the state of this PR. What do you think?

@sdrave
Copy link
Member Author

sdrave commented Mar 14, 2019

BTW, I decided against introducing a ShiftedOperator in this PR but added a shift parameter to _assemble_lincomb. I would propose to introduce ShiftedOperator in a separate PR if needed.

@pmli
Copy link
Member

pmli commented Mar 14, 2019

One of my favourite tests fails again.

>>> from pymor.basic import *
>>> I = IdentityOperator(NumpyVectorSpace(2))
>>> v = I.source.ones()
>>> (I + I).apply_inverse(v)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/path/pymor/src/pymor/operators/constructions.py", line 166, in apply_inverse
    return super().apply_inverse(V, mu=mu, least_squares=least_squares)
  File "/path/pymor/src/pymor/operators/basic.py", line 101, in apply_inverse
    assembled_op = self.assemble(mu)
  File "/path/pymor/src/pymor/operators/constructions.py", line 129, in assemble
    name=self.name + '_assembled')
  File "/path/pymor/src/pymor/algorithms/lincomb.py", line 48, in assemble_lincomb
    return AssembleLincombRules(tuple(coefficients), solver_options, name).apply(tuple(operators))
  File "/path/pymor/src/pymor/algorithms/rules.py", line 269, in apply
    result = r.action(self, obj)
  File "/path/pymor/src/pymor/algorithms/lincomb.py", line 168, in action_call_assemble_lincomb_method
    op = ops_without_id[0]._assemble_lincomb(ops_without_id, self.coefficients, shift=id_coeff,
IndexError: list index out of range

@renefritze
Copy link
Member

Maybe add that as an actual test in https://github.com/pymor/pymor/blob/master/src/pymortests/operators.py ?

@sdrave
Copy link
Member Author

sdrave commented Mar 15, 2019

That issue should be fixed. Anything else?

@pmli
Copy link
Member

pmli commented Mar 15, 2019

Technically, it looks ok to me. I'm a bit unsure about the name shift for the argument in _assemble_lincomb. An operator could be shifted by anything, not just IdentityOperator... How about renaming it to id_op_coef, or something similar?

@sdrave
Copy link
Member Author

sdrave commented Mar 15, 2019

Aren't you guys usually speaking of solving shifted equation systems where you implicitly mean 'shifted by a multiple of the identity'. I have nothing against renaming the argument, but how abount 'identity_shift'?

@pmli
Copy link
Member

pmli commented Mar 15, 2019

Aren't you guys usually speaking of solving shifted equation systems where you implicitly mean 'shifted by a multiple of the identity'.

It can mean 'shifted by a multiple of the mass matrix' (s E - A in IRKA, A + p E in ADI) and the mass matrix can be almost anything.

I have nothing against renaming the argument, but how abount 'identity_shift'?

That also sounds good.

@sdrave
Copy link
Member Author

sdrave commented Mar 15, 2019

Right, forgot about the mass matrix .. - I have renamed the parameter to identity_shift.

@sdrave sdrave merged commit 8b9c05b into master Mar 15, 2019
@sdrave sdrave deleted the free_assemble_lincomb branch March 15, 2019 19:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:change Change in existing functionality pr:new-feature Introduces a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants