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

Add methods to models.iosys for exporting matrices #1309

Merged
merged 13 commits into from Jun 4, 2021
Merged

Conversation

pmli
Copy link
Member

@pmli pmli commented May 17, 2021

This PR adds to_* methods to LTIModel and SecondOrderModel, tests for from_* and to_* methods, and output methods to tools.io. The tests should cover most of what is not covered by #1300 in models.iosys. I'm not sure how the CI will react to creating temporary files; I guess we'll see.

@pmli pmli added the pr:new-feature Introduces a new feature label May 17, 2021
@pmli pmli added this to the 2021.1 milestone May 17, 2021
@codecov
Copy link

codecov bot commented May 24, 2021

Codecov Report

Merging #1309 (49e3a3b) into main (19c2c5b) will increase coverage by 0.34%.
The diff coverage is 91.11%.

Impacted Files Coverage Δ
src/pymor/tools/io.py 74.07% <73.61%> (+18.83%) ⬆️
src/pymor/models/iosys.py 57.88% <93.10%> (+7.95%) ⬆️
src/pymortests/iosys_save_load.py 100.00% <100.00%> (ø)
src/pymortests/tools.py 96.42% <100.00%> (+0.40%) ⬆️
src/pymortests/vectorarray.py 91.90% <0.00%> (-0.48%) ⬇️
src/pymor/vectorarrays/numpy.py 84.96% <0.00%> (-0.24%) ⬇️
src/pymortests/mpi_run_demo_tests.py 89.74% <0.00%> (+5.12%) ⬆️

Copy link
Member

@sdrave sdrave left a comment

Choose a reason for hiding this comment

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

LGTM. However, could you remove the type annotations for now? I don't think it is quite clear at the moment if/when we will have type annotations in pyMOR ..

@pmli
Copy link
Member Author

pmli commented May 31, 2021

LGTM. However, could you remove the type annotations for now? I don't think it is quite clear at the moment if/when we will have type annotations in pyMOR ..

I removed the type annotations now.

I also realized that probably save_matrix should be tested as much as load_matrix is. Currently, files in pymortests/data/matrices are used to test load_matrix. How about removing those and testing save_matrix and load_matrix by a round trip as for iosys models?

@renefritze
Copy link
Member

I like the current setup as it forces one to consider backwards compat. Maybe we could use the saved (or a new set, since that'll be easier) compare files also for a check if save reproduces them?

@pmli pmli requested a review from sdrave June 3, 2021 07:13
@pmli pmli merged commit 2bd9055 into main Jun 4, 2021
@pmli pmli deleted the iosys-to-methods branch June 4, 2021 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:new-feature Introduces a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants