Skip to content

Neural networks for instationary problems#1120

Merged
sdrave merged 14 commits into
pymor:masterfrom
HenKlei:feature-nns-instationary
Nov 4, 2020
Merged

Neural networks for instationary problems#1120
sdrave merged 14 commits into
pymor:masterfrom
HenKlei:feature-nns-instationary

Conversation

@HenKlei

@HenKlei HenKlei commented Oct 13, 2020

Copy link
Copy Markdown
Member

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

HenKlei commented Oct 13, 2020

Copy link
Copy Markdown
Member Author

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

@renefritze

renefritze commented Oct 13, 2020

Copy link
Copy Markdown
Member

Did you see these?

@HenKlei

HenKlei commented Oct 13, 2020

Copy link
Copy Markdown
Member Author

Did you see these?

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

@renefritze

renefritze commented Oct 13, 2020

Copy link
Copy Markdown
Member

Did you see these?

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

Indent was off-by-one.

@HenKlei

HenKlei commented Oct 13, 2020

Copy link
Copy Markdown
Member Author

Indent was off-by-one.

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

@renefritze

renefritze commented Oct 13, 2020

Copy link
Copy Markdown
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

codecov Bot commented Oct 13, 2020 •
edited
Loading

Copy link
Copy Markdown

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%) ⬆️

sdrave
sdrave requested changes Oct 26, 2020
View reviewed changes

@sdrave sdrave left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Only some minor things ...

Comment thread src/pymor/models/neural_network.py Outdated
Comment thread src/pymor/models/neural_network.py Outdated
Comment thread src/pymor/models/neural_network.py Outdated
Comment thread src/pymor/reductors/neural_network.py Outdated
Comment thread src/pymor/models/neural_network.py Outdated
Comment thread src/pymor/reductors/neural_network.py Outdated
@HenKlei

HenKlei commented Oct 26, 2020

Copy link
Copy Markdown
Member Author

@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

sdrave commented Oct 26, 2020

Copy link
Copy Markdown
Member

@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

HenKlei commented Oct 27, 2020

Copy link
Copy Markdown
Member Author

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 reviewed Oct 28, 2020
View reviewed changes
Comment thread src/pymor/reductors/neural_network.py Outdated
@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
sdrave
sdrave approved these changes Nov 3, 2020
View reviewed changes

@sdrave sdrave left a comment

Copy link
Copy Markdown
Member

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.

Comment thread src/pymor/reductors/neural_network.py Outdated
Comment thread src/pymor/reductors/neural_network.py Outdated
Comment thread src/pymor/reductors/neural_network.py Outdated
Comment thread src/pymor/reductors/neural_network.py Outdated
Comment thread src/pymor/reductors/neural_network.py Outdated
@HenKlei

HenKlei commented Nov 4, 2020

Copy link
Copy Markdown
Member Author

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 and others added 11 commits November 4, 2020 09:10
@HenKlei
[neural-networks] first implementation of reductor for instationary p…
9f7c5eb 
…roblems
@HenKlei
[neural-networks] improved reductor and model for instationary problems
11524b8
@HenKlei
[neural-networks] fixed bugs in creation of training set
b3c6f15
@HenKlei
[neural-networks] restructure instationary reductor to use functions …
46f895c 
…from base class
@HenKlei
[neural-networks] added paper describing the method for instationary …
da4612d 
…problems
@HenKlei
[AUTHORS] updated authors
f7104e7
@HenKlei
[pymortests] added demo using NeuralNetworkInstationaryReductor to tests
b5e0ffa
@HenKlei
[neural-networks] small fixes after rebasing on master
a949342
@renefritze @HenKlei
[neural-networks] fix docstring indent
3d83729
@HenKlei
[neural-networks] several small fixes mentioned in review
35cf59c
@HenKlei
[neural-networks] avoid keeping validation snapshots in memory
f4636f6
@HenKlei HenKlei force-pushed the feature-nns-instationary branch from 8f7ee4f to 9402b0d Compare November 4, 2020 08:11
@HenKlei

HenKlei commented Nov 4, 2020

Copy link
Copy Markdown
Member Author

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
@HenKlei HenKlei mentioned this pull request Nov 20, 2020
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sdrave sdrave sdrave approved these changes

@renefritze renefritze renefritze approved these changes

Assignees

No one assigned

Labels

pr:new-feature Introduces a new feature

Projects

None yet

Milestone

2020.2

Development

Successfully merging this pull request may close these issues.

3 participants

@HenKlei @renefritze @sdrave