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

Support automatic conversion of InstationaryDiscretization to LTISystem #497

Merged
merged 8 commits into from Dec 6, 2018

Conversation

Projects
None yet
2 participants
@sdrave
Copy link
Member

sdrave commented Dec 3, 2018

This add a to_lti method to InstationaryDiscretization. The d.rhs is chosen as B matrix.

Since InstationaryDiscretization has no constraints regarding space ids I decided to lift the requirement that state/input/output spaces in LTISystem have fixed space ids. Now, the only requirement is that the state space has a different id from both the input space and the output space. I expect the to be useful also in other situations where one considers multiple systems at the same time.

The new to_lti method is now used in the heat.py demo. One could also decide to showcase a 2d problem.

@sdrave sdrave requested a review from pmli Dec 3, 2018

@sdrave sdrave added this to the 0.5 milestone Dec 3, 2018

@sdrave

This comment has been minimized.

Copy link
Member Author

sdrave commented Dec 3, 2018

Fixes #454.

Show resolved Hide resolved src/pymordemos/heat.py Outdated
@pmli
Copy link
Member

pmli left a comment

It looks very nice. I like the use of more abstract input_space and output_space.

Only now I realized that 'STATE' and state_space are not used completely correctly. As far as I understand, the state is defined as the information necessary to simulate a system forward from some time instant (basically, the initial condition). So, for LTISystems, the vector x really is the state, but for SecondOrderSystem the state is x and its derivative. For delay systems, the state are all the values of x over some time interval.
But, I'm not sure if there is a unified terminology for x ("position" would make sense for second-order systems), so maybe also calling it "state" is ok for now...

Changing the heat demo example from 1D to 2D sound good to me.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Dec 4, 2018

Codecov Report

Merging #497 into master will increase coverage by 0.33%.
The diff coverage is 66.66%.

Impacted Files Coverage Δ
src/pymor/discretizers/cg.py 90.98% <ø> (+4.91%) ⬆️
src/pymor/reductors/sobt.py 81.43% <0%> (ø) ⬆️
src/pymor/reductors/basic.py 85% <0%> (ø) ⬆️
src/pymor/discretizations/iosys.py 64.77% <68.18%> (+0.69%) ⬆️
src/pymor/discretizations/basic.py 85.93% <76.92%> (-1.02%) ⬇️
src/pymor/operators/block.py 65.87% <0%> (+0.33%) ⬆️
src/pymor/operators/numpy.py 81.4% <0%> (+0.5%) ⬆️
src/pymor/grids/interfaces.py 84.07% <0%> (+1.76%) ⬆️
src/pymor/operators/cg.py 91.89% <0%> (+4.24%) ⬆️
... and 1 more
@codecov

This comment has been minimized.

Copy link

codecov bot commented Dec 4, 2018

Codecov Report

Merging #497 into master will increase coverage by 0.38%.
The diff coverage is 72.28%.

Impacted Files Coverage Δ
src/pymor/discretizers/cg.py 90.98% <ø> (+4.91%) ⬆️
src/pymor/reductors/sobt.py 81.43% <0%> (ø) ⬆️
src/pymor/reductors/basic.py 85% <0%> (ø) ⬆️
src/pymor/discretizations/iosys.py 65.07% <74.62%> (+1%) ⬆️
src/pymor/discretizations/basic.py 85.93% <76.92%> (-1.02%) ⬇️
src/pymor/operators/block.py 65.87% <0%> (+0.33%) ⬆️
src/pymor/operators/numpy.py 81.4% <0%> (+0.5%) ⬆️
src/pymor/grids/interfaces.py 84.07% <0%> (+1.76%) ⬆️
src/pymor/operators/cg.py 93.05% <0%> (+5.4%) ⬆️
... and 1 more
@sdrave

This comment has been minimized.

Copy link
Member Author

sdrave commented Dec 4, 2018

I see. Probably, it doesn't really make sense to have a state_space for an arbitrary InputOutputSystem in the first place. The only reason for defining it there is that it is used in InputOutputSystem.__add__. One simple solution would be to just not use it there, i.e. basically remove the NotImplementedError cases.

@pmli

This comment has been minimized.

Copy link
Member

pmli commented Dec 4, 2018

Maybe it makes sense to add an InputStateOutputSystem class, which would inherit from InputOutputSystem ("input-output system" and "input-state-output system" is common terminology, AFAIK). TransferFunction would still inherit from InputOutputSystem, but all others (LTISystem, SecondOrderSystem, ...) would inherit from InputStateOutputSystem. The __add__ method would be moved from InputOutputSystem to InputStateOutputSystem. TransferFunction.__add__ would then not work with operators, but with eval_tf and eval_dtf methods. At some point, TransferFunction could have its own subclasses, e.g., for pole-residue and barycentric form.

@pmli

This comment has been minimized.

Copy link
Member

pmli commented Dec 4, 2018

@sdrave Using InstationaryDiscretization.to_lti, is there a way to define an LTISystem with multiple input and/or multiple outputs?

@sdrave

This comment has been minimized.

Copy link
Member Author

sdrave commented Dec 4, 2018

Regarding InputStateOutputSystem: I like the approach. However, we would still have the issue that 'state' might indeed be misleading for higher order or delay systems. I did some research but could not find any better term. Are you sure you are ok with 'state'?

@pmli

This comment has been minimized.

Copy link
Member

pmli commented Dec 4, 2018

I guess, either way, we need an initial_condition_space? This would then not cause confusion with state_space.
Also, I think (E, A, B, C, D) systems, for which x is the state, are so common that calling x the 'state' in general is not too misleading. If we find a better term, we could change it.

@sdrave

This comment has been minimized.

Copy link
Member Author

sdrave commented Dec 5, 2018

I have just added a commit which introduces InputStateOutputSystem. Is this ready to be merged?

@pmli

pmli approved these changes Dec 5, 2018

Copy link
Member

pmli left a comment

LGTM

@sdrave sdrave merged commit 59d0ce2 into master Dec 6, 2018

4 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@sdrave sdrave deleted the instat_to_lti branch Dec 6, 2018

@pmli pmli added the sys-mor label Jan 19, 2019

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