Moebius Transform Operator and Tustin's method for LTI-Systems by artpelling · Pull Request #1614 · pymor/pymor

artpelling · 2022-04-20T10:29:38Z

I have this code here that I would like to integrate in order to have Bilinear Transformations of LTI-systems similarly to Matlabs c2d and d2c commands. So far I have implemented Tustin's method and I am looking to extend this to higher order approximations in the future.

I am particularly interested in arbitrary Moebius substitutions of the frequency argument (not only the bilinear transformation) which is why I chose a more functional approach and implemented a MoebiusTransform operator directly. Maybe this can be used elsewhere in the future (I have added the CayleyTransform as an inspiration).

I would love to get some feedback, IMO the method LTIModel.moebius_substitution could use some improvement in both naming and the code. The method substitutes the frequency argument omega by a MoebiusTransform (please see https://ieeexplore.ieee.org/document/489281/ for reference). I feel like I am not using pyMORs structure optimally in the current implementation.

The docstrings are sparse and sketchy, I will obviously add proper ones and tests at a later stage.

codecov · 2022-04-20T11:20:33Z

Codecov Report

Merging #1614 (fa218ff) into main (90ca18d) will decrease coverage by 11.27%.
The diff coverage is 90.08%.

Impacted Files	Coverage Δ
src/pymor/algorithms/to_matrix.py	`88.23% <77.27%> (-10.43%)`	⬇️
src/pymor/models/transforms.py	`92.53% <92.53%> (ø)`
src/pymor/models/iosys.py	`80.66% <93.75%> (+0.49%)`	⬆️
src/pymordemos/fenics_nonlinear.py	`0.00% <0.00%> (-97.30%)`	⬇️
src/pymordemos/neural_networks_instationary.py	`0.00% <0.00%> (-97.02%)`	⬇️
src/pymordemos/neural_networks.py	`0.00% <0.00%> (-96.43%)`	⬇️
src/pymordemos/neural_networks_fenics.py	`0.00% <0.00%> (-92.96%)`	⬇️
src/pymor/reductors/neural_network.py	`0.51% <0.00%> (-92.21%)`	⬇️
src/pymor/tools/io/vtk.py	`3.44% <0.00%> (-91.38%)`	⬇️
src/pymor/models/neural_network.py	`2.27% <0.00%> (-89.78%)`	⬇️
... and 74 more

pymor-lab-hub-bridge · 2022-04-27T15:11:56Z

Hey @artpelling it looks like this PR touches our CI config (and you are not in my contributor safelist), therefore I am not starting a gitlab-ci build for it and it will not be mergeable.

pymor-lab-hub-bridge · 2022-04-28T15:13:26Z

Hey @artpelling it looks like this PR touches our CI config (and you are not in my contributor safelist), therefore I am not starting a gitlab-ci build for it and it will not be mergeable.

artpelling · 2022-04-28T15:19:10Z

I have somehow messed up the history again. @pmli if you are okay with the current state, I can rebase on main, squash everything into one commit and convert this to a non-draft PR.

pmli · 2022-04-28T20:27:46Z

I have somehow messed up the history again. @pmli if you are okay with the current state, I can rebase on main, squash everything into one commit and convert this to a non-draft PR.

Rebasing would also make it easier to review. You don't have to squash it into one commit, e.g., using one commit per file changed would make the git history nicer (also, we often use git commit messages of the form [module] short message, but we do not really enforce it).

pmli

It looks to me to be in a good shape. What is left are docstrings and some unit tests.

artpelling · 2022-05-02T20:19:12Z

I renamed MoebiusTransform to MoebiusTransformation, because apparently the former refers to Möbius' inversion formula. This leads to inconsistent but correct naming of the derived classes.

pmli · 2022-05-03T18:47:25Z

It would be good to add tests for Moebius transformations and maybe also for to_discrete and to_continuous methods.

Also, now that it's settled that these transformations are not Operators, maybe __mul__ instead of __matmul__ is better? Or would that be confused with the elementwise product of functions?

pymor-lab-hub-bridge · 2022-05-30T13:43:47Z

Hey @artpelling it looks like this PR touches our CI config (and you are not in my contributor safelist), therefore I am not starting a gitlab-ci build for it and it will not be mergeable.

pymor-lab-hub-bridge · 2022-05-31T08:46:31Z

Hey @artpelling it looks like this PR touches our CI config (and you are not in my contributor safelist), therefore I am not starting a gitlab-ci build for it and it will not be mergeable.

pymor-lab-hub-bridge · 2022-05-31T13:33:22Z

Hey @artpelling it looks like this PR touches our CI config (and you are not in my contributor safelist), therefore I am not starting a gitlab-ci build for it and it will not be mergeable.

pymor-lab-hub-bridge · 2022-05-31T15:55:55Z

Hey @artpelling it looks like this PR touches our CI config (and you are not in my contributor safelist), therefore I am not starting a gitlab-ci build for it and it will not be mergeable.

renefritze · 2022-06-07T09:40:50Z

I've add you to the safelist now @artpelling , so this message should disappear with the next push/commit.

Co-authored-by: Petar Mlinarić <petar.mlinaric@gmail.com>

…perator" This reverts commit f525e4b.

sdrave · 2022-07-05T16:14:12Z

@artpelling, the problem with the failing test was that you are not allowed to call as_range_array on an Operator which has something different than a NumpyVectorSpace as source. I have moved the fallback in to_matrix to an additional as_array action. As this should also work for the children in the Concatenation, I do not think it is necessary to catch NoMatchingRuleError.

Further I have reverted your streamlined version of the matrix-matrix products as it seems to neglect one optimization for sparse-dense products.

pmli reviewed Apr 21, 2022

View reviewed changes

Comment thread src/pymor/operators/numpy.py Outdated

Comment thread src/pymor/models/iosys.py Outdated

Comment thread src/pymor/models/iosys.py Outdated

pmli reviewed Apr 21, 2022

View reviewed changes

Comment thread src/pymor/algorithms/to_matrix.py Outdated

artpelling commented Apr 22, 2022

View reviewed changes

Comment thread src/pymor/models/iosys.py Outdated

artpelling force-pushed the moebius-transform branch 2 times, most recently from c458e48 to 14a204b Compare April 27, 2022 15:11

pmli reviewed Apr 27, 2022

View reviewed changes

Comment thread src/pymor/models/transforms.py Outdated

Comment thread src/pymor/models/transforms.py Outdated

pmli reviewed Apr 28, 2022

View reviewed changes

Comment thread src/pymor/models/transforms.py Outdated

artpelling force-pushed the moebius-transform branch from cff0d12 to 429a78a Compare May 2, 2022 17:18

artpelling marked this pull request as ready for review May 2, 2022 17:46

pmli added the pr:new-feature Introduces a new feature label May 2, 2022

pmli added this to the 2022.1 milestone May 2, 2022

pmli reviewed May 2, 2022

View reviewed changes

Comment thread src/pymor/algorithms/to_matrix.py Outdated

artpelling force-pushed the moebius-transform branch from b5e2427 to 2bbf866 Compare May 2, 2022 20:16

pmli reviewed May 3, 2022

View reviewed changes

artpelling force-pushed the moebius-transform branch from 22c5c0a to 1b597c4 Compare May 30, 2022 12:59

artpelling force-pushed the moebius-transform branch 2 times, most recently from 22c5c0a to aa5e13e Compare May 31, 2022 08:05

github-actions Bot added the infrastructure Buildsystem, packages and CI label May 31, 2022

artpelling and others added 23 commits July 4, 2022 14:00

rename MoebiusTransform to MoebiusTransformation

bd6854f

add docstrings

553077c

remove to_matrix and normalize from Moebius derived transforms

048f85e

linting and typo fix

c0bf0bf

[AUTHORS.md] add changes

a1a6fcd

Apply suggestions from code review

3bab7f0

Co-authored-by: Petar Mlinarić <petar.mlinaric@gmail.com>

add missing Returns to docstrings

cc5974b

remove complex casting and obsolete dim argument

01901ac

change names of derived classes for consistency

31200ba

add License string

1ec870d

fix normalization

c3b0130

[pymortests.transforms] add tests for transforms

86f6352

[pymortests.transforms] add basic test for LTI conversions

e570dc4

Apply suggestions from code review

8de7863

Co-authored-by: Petar Mlinarić <petar.mlinaric@gmail.com>

[models.transforms] prettify __str__ method

52481ab

change syntax to fix docs

228bc66

change normalization

36ba7f1

change normalization (again)

79a796a

[models.transforms] make __str__ even prettier

025a898

[docs] add substitutions

03b0110

[docs] fix substitutions

7258977

[iosys] cast sampling_time to float

f75f5c4

[transforms] assert x is Number

a61db8a

artpelling force-pushed the moebius-transform branch from fcc7988 to a61db8a Compare July 4, 2022 12:00

renefritze assigned sdrave Jul 5, 2022

sdrave added 2 commits July 5, 2022 17:15

Revert "[algorithms.to_matrix] modify MatchingRule for ConcatenationO…

46a99a9

…perator" This reverts commit f525e4b.

[to_matrix] add as_array as a fallback

fa218ff

pmli approved these changes Jul 11, 2022

View reviewed changes

renefritze merged commit f968478 into pymor:main Jul 11, 2022

Conversation

artpelling commented Apr 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Apr 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pymor-lab-hub-bridge Bot commented Apr 27, 2022

Uh oh!

Uh oh!

Uh oh!

pymor-lab-hub-bridge Bot commented Apr 28, 2022

Uh oh!

artpelling commented Apr 28, 2022

Uh oh!

Uh oh!

pmli commented Apr 28, 2022

Uh oh!

pmli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

artpelling commented May 2, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pmli commented May 3, 2022

Uh oh!

pymor-lab-hub-bridge Bot commented May 30, 2022

Uh oh!

pymor-lab-hub-bridge Bot commented May 31, 2022

Uh oh!

pymor-lab-hub-bridge Bot commented May 31, 2022

Uh oh!

pymor-lab-hub-bridge Bot commented May 31, 2022

Uh oh!

renefritze commented Jun 7, 2022

Uh oh!

sdrave commented Jul 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

artpelling commented Apr 20, 2022 •

edited

Loading

codecov Bot commented Apr 20, 2022 •

edited

Loading