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

Neural networks for instationary problems #1120

Merged
merged 14 commits into from
Nov 4, 2020

Conversation

HenKlei
Copy link
Member

@HenKlei HenKlei commented Oct 13, 2020

This pull request adds a NeuralNetworkInstationaryReductor that handles instationary problems. Furthermore, a demo solving Burgers' equation is implemented and added to pymortests.

@renefritze renefritze added the pr:new-feature Introduces a new feature label Oct 13, 2020
@HenKlei
Copy link
Member Author

HenKlei commented Oct 13, 2020

@renefritze Do you see what is wrong with the documentation? I can't figure out where the warnings come from.

@renefritze
Copy link
Member

Did you see these?

@HenKlei
Copy link
Member Author

HenKlei commented Oct 13, 2020

Did you see these?

Yes, but I do not really understand what the problem is.

@renefritze
Copy link
Member

Did you see these?

Yes, but I do not really understand what the problem is.

Indent was off-by-one.

@HenKlei
Copy link
Member Author

HenKlei commented Oct 13, 2020

Indent was off-by-one.

Thanks, I don't know why I did not see that...

@renefritze
Copy link
Member

Indent was off-by-one.

Thanks, I don't know why I did not see that...

Cause it's literally only two spaces and the warning message from docutils is effectively emitted for the transformed text.

I have indent guides on in atom, that made it easier to spot for me.

@codecov
Copy link

codecov bot commented Oct 13, 2020

Codecov Report

Merging #1120 into master will increase coverage by 0.11%.
The diff coverage is 98.18%.

Impacted Files Coverage Δ
src/pymortests/demos.py 86.89% <ø> (ø)
src/pymor/models/neural_network.py 94.33% <95.00%> (-0.11%) ⬇️
src/pymordemos/neural_networks_instationary.py 97.72% <97.72%> (ø)
src/pymor/reductors/neural_network.py 92.82% <100.00%> (+2.82%) ⬆️
src/pymor/bindings/ngsolve.py 84.32% <0.00%> (-0.75%) ⬇️
src/pymor/vectorarrays/numpy.py 84.48% <0.00%> (-0.24%) ⬇️
src/pymor/vectorarrays/interface.py 82.27% <0.00%> (+0.42%) ⬆️

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.

Only some minor things ...

src/pymor/models/neural_network.py Outdated Show resolved Hide resolved
src/pymor/models/neural_network.py Outdated Show resolved Hide resolved
src/pymor/models/neural_network.py Outdated Show resolved Hide resolved
src/pymor/reductors/neural_network.py Outdated Show resolved Hide resolved
src/pymor/models/neural_network.py Outdated Show resolved Hide resolved
src/pymor/reductors/neural_network.py Outdated Show resolved Hide resolved
@HenKlei
Copy link
Member Author

HenKlei commented Oct 26, 2020

@sdrave Thanks for taking a look. In the meantime, @ftalbrecht started to restructure the implementation of our neural network-based reductor and related stuff. At the moment, I am preparing a PR for these changes. Maybe it makes more sense to first do the restructuring and implement the instationary case afterwards.

@sdrave
Copy link
Member

sdrave commented Oct 26, 2020

@sdrave Thanks for taking a look. In the meantime, @ftalbrecht started to restructure the implementation of our neural network-based reductor and related stuff. At the moment, I am preparing a PR for these changes. Maybe it makes more sense to first do the restructuring and implement the instationary case afterwards.

If you think the new PR will be ready soonish, then yes, it probably makes sense to close this PR for now. On the other hand, this is almost good to merge, and it would be nice to have it in the next release. You decide what to do ..

@HenKlei HenKlei force-pushed the feature-nns-instationary branch from 3e4f590 to 58cbb6c Compare October 27, 2020 16:08
@HenKlei
Copy link
Member Author

HenKlei commented Oct 27, 2020

If you think the new PR will be ready soonish, then yes, it probably makes sense to close this PR for now. On the other hand, this is almost good to merge, and it would be nice to have it in the next release. You decide what to do ..

The other PR will still take some time. I am also not completely convinced about the new structure introduced there. We need to further discuss that at some point.

Consequently, I would propose to finish this PR first, since there were quite a few questions regarding neural networks for instationary problems at the pyMOR course as well. Some people seemed to be interested in that.

@sdrave sdrave added this to the 2020.2 milestone Oct 29, 2020
@renefritze renefritze force-pushed the feature-nns-instationary branch from ca35785 to cee7c49 Compare November 3, 2020 16:44
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.

I just noticed that you always leave two empty lines between class members. PEP8 states that it should only be 1 (two lines between global definitions.) So probably my linter would complain.

src/pymor/reductors/neural_network.py Outdated Show resolved Hide resolved
src/pymor/reductors/neural_network.py Outdated Show resolved Hide resolved
src/pymor/reductors/neural_network.py Outdated Show resolved Hide resolved
src/pymor/reductors/neural_network.py Outdated Show resolved Hide resolved
src/pymor/reductors/neural_network.py Outdated Show resolved Hide resolved
@HenKlei
Copy link
Member Author

HenKlei commented Nov 4, 2020

I just noticed that you always leave two empty lines between class members. PEP8 states that it should only be 1 (two lines between global definitions.) So probably my linter would complain.

Oh, I'm sorry, I will fix that!

@HenKlei HenKlei force-pushed the feature-nns-instationary branch from 8f7ee4f to 9402b0d Compare November 4, 2020 08:11
@HenKlei
Copy link
Member Author

HenKlei commented Nov 4, 2020

I think this can be merged if the checks pass.

@sdrave sdrave merged commit b81e178 into pymor:master Nov 4, 2020
@HenKlei HenKlei deleted the feature-nns-instationary branch November 20, 2020 09:13
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.

3 participants