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.

Sign up for GitHub

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

Jump to bottom

Fix return_fpeak handling in pymor.models.iosys #2134

Merged
merged 3 commits into from Aug 31, 2023
Merged

Fix return_fpeak handling in pymor.models.iosys #2134

merged 3 commits into from Aug 31, 2023

Conversation

peoe
Copy link
Contributor

@peoe peoe commented Jul 27, 2023

This is a proposed fix to issue #2133.

@github-actions
Copy link
Contributor

Mirroring external branch in external_pr_2134

@pmli pmli added the pr:fix Fixes a bug label Aug 7, 2023
@pmli pmli added this to the 2023.2 milestone Aug 7, 2023
@pmli pmli linked an issue Aug 7, 2023 that may be closed by this pull request
LTIModel.hinf_norm() always raises TypeError with additional argument return_fpeak #2133
Closed
pmli
pmli reviewed Aug 7, 2023
View reviewed changes
Copy link
Member

@pmli pmli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!
Would you also add tests? This method seems to be causing many issues.

@peoe
Copy link
Contributor Author

peoe commented Aug 30, 2023

I added a simple test in pymortests/models/iosys_norm.py. If you have more ideas what to test for or where to place this file, please let me know!

pmli
pmli reviewed Aug 30, 2023
View reviewed changes
Copy link
Member

@pmli pmli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. The new test file should include the copyright header and probably also the line pytestmark = pytest.mark.builtin as in here for the fenics test to pass.

pmli
pmli approved these changes Aug 31, 2023
View reviewed changes
Copy link
Member

@pmli pmli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@pmli pmli added this pull request to the merge queue Aug 31, 2023
Merged via the queue into pymor:main with commit 8ff5cf6 Aug 31, 2023
18 checks passed
@github-actions
Copy link
Contributor

Local mirror branch external_pr_2134 deleted.

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

@pmli pmli pmli approved these changes

Assignees
No one assigned
Labels
pr:fix Fixes a bug
Projects
None yet
Milestone
2023.2
Development

Successfully merging this pull request may close these issues.

LTIModel.hinf_norm() always raises TypeError with additional argument return_fpeak
2 participants
@peoe @pmli