Skip to content

Energy product in elliptic discretizer (Continuation)#1008

Merged
sdrave merged 24 commits into
pymor:masterfrom
TiKeil:energy-product-in-elliptic-discretizer
Aug 25, 2020
Merged

Energy product in elliptic discretizer (Continuation)#1008
sdrave merged 24 commits into
pymor:masterfrom
TiKeil:energy-product-in-elliptic-discretizer

Conversation

@TiKeil

@TiKeil TiKeil commented Jul 9, 2020

Copy link
Copy Markdown

In this PR I continue #875 . I added the requested changes and changed a demo very briefly. Let me know if there is additional work to do for this.

@TiKeil

TiKeil commented Jul 9, 2020

Copy link
Copy Markdown
Author

@renefritze, As it shows in https://dev.azure.com/pymor/pymor/_build/results?buildId=2757&view=logs&j=b2cbc7ea-7585-56c2-8847-7376d77354b6&t=1f352729-2dd2-595d-2a18-f5a7564f328f, it seems that af2c20c was not enough for changing the args in the elliptic2 demo. Could you help out where else I need to edit it?

@renefritze

Copy link
Copy Markdown
Member

The problem isn't in the commit you linked, I think. But in the changing of the docopt string for the demo. To get more insight you can locally execute just that demo with -h and you'll probably see a similar error. My guess would be that the colons confuse docopt's parsing.

@renefritze renefritze added the pr:new-feature Introduces a new feature label Jul 10, 2020
@sdrave

sdrave commented Jul 16, 2020

Copy link
Copy Markdown
Member

@TiKeil, can you rebase on master?

@sdrave sdrave added this to the 2020.2 milestone Jul 16, 2020
@TiKeil TiKeil force-pushed the energy-product-in-elliptic-discretizer branch from 99f6e3f to a1e4d62 Compare July 16, 2020 10:27
@TiKeil

TiKeil commented Jul 16, 2020

Copy link
Copy Markdown
Author

Does anybody understand whats wrong with the tests again?

@renefritze

Copy link
Copy Markdown
Member

@TiKeil

TiKeil commented Jul 17, 2020

Copy link
Copy Markdown
Author

Thanks a lot. Could have found this by myself : /

@TiKeil TiKeil force-pushed the energy-product-in-elliptic-discretizer branch from a1e4d62 to c3ed6bc Compare July 17, 2020 10:09
Comment thread src/pymordemos/elliptic2.py Outdated
@TiKeil

TiKeil commented Jul 17, 2020

Copy link
Copy Markdown
Author

To get more insight you can locally execute just that demo with -h and you'll probably see a similar error.

This is not showing an error.

My guess would be that the colons confuse docopt's parsing.

Do you see any difference to the docstring in https://github.com/pymor/pymor/blob/master/src/pymordemos/elliptic.py ?

@renefritze

Copy link
Copy Markdown
Member

To get more insight you can locally execute just that demo with -h and you'll probably see a similar error.

This is not showing an error.

My guess would be that the colons confuse docopt's parsing.

Do you see any difference to the docstring in https://github.com/pymor/pymor/blob/master/src/pymordemos/elliptic.py ?

Yes: e699c01 😉

If this works, the commit should be squashed before the merge.

@TiKeil TiKeil force-pushed the energy-product-in-elliptic-discretizer branch from e699c01 to fa4b578 Compare July 17, 2020 12:33
@codecov

codecov Bot commented Jul 17, 2020

Copy link
Copy Markdown

Codecov Report

Merging #1008 into master will decrease coverage by 0.03%.
The diff coverage is 65.51%.

Impacted Files Coverage Δ
src/pymor/discretizers/builtin/cg.py 89.93% <65.51%> (-1.32%) ⬇️
src/pymor/bindings/ngsolve.py 84.55% <0.00%> (-0.74%) ⬇️
src/pymor/vectorarrays/list.py 83.77% <0.00%> (-0.57%) ⬇️
src/pymor/vectorarrays/numpy.py 84.55% <0.00%> (+0.24%) ⬆️

@TiKeil

TiKeil commented Jul 17, 2020

Copy link
Copy Markdown
Author

Thanks @renefritze for fixing.
I think this should be ready now.

Comment thread AUTHORS.md Outdated
Comment thread src/pymor/discretizers/builtin/cg.py Outdated
Comment thread src/pymor/discretizers/builtin/cg.py Outdated
Comment thread src/pymor/discretizers/builtin/cg.py Outdated
Comment thread src/pymor/discretizers/builtin/cg.py Outdated
Comment thread src/pymor/discretizers/builtin/cg.py Outdated
Comment thread src/pymordemos/elliptic2.py Outdated
Comment thread src/pymordemos/elliptic2.py Outdated
@TiKeil TiKeil force-pushed the energy-product-in-elliptic-discretizer branch 2 times, most recently from 3a691bd to 80285ba Compare August 3, 2020 17:47
@TiKeil

TiKeil commented Aug 3, 2020

Copy link
Copy Markdown
Author

tried to address your points as much as possible.

@TiKeil TiKeil force-pushed the energy-product-in-elliptic-discretizer branch from 80285ba to 55d2192 Compare August 4, 2020 09:14
@TiKeil TiKeil force-pushed the energy-product-in-elliptic-discretizer branch from 55d2192 to bc32ab6 Compare August 12, 2020 13:53
Comment thread src/pymor/discretizers/builtin/cg.py Outdated
Comment thread src/pymor/discretizers/builtin/cg.py Outdated
Comment thread src/pymordemos/elliptic2.py Outdated
Comment thread src/pymordemos/elliptic2.py Outdated
Comment thread src/pymordemos/elliptic2.py Outdated
Comment thread src/pymordemos/elliptic2.py Outdated
@TiKeil TiKeil force-pushed the energy-product-in-elliptic-discretizer branch from f44847d to cf7ef1e Compare August 18, 2020 12:28
@TiKeil

TiKeil commented Aug 18, 2020

Copy link
Copy Markdown
Author

Thanks @sdrave, I think I addressed all your points.

@TiKeil TiKeil force-pushed the energy-product-in-elliptic-discretizer branch from cf7ef1e to c0a8984 Compare August 24, 2020 15:25
Comment thread src/pymordemos/elliptic2.py Outdated
Comment on lines +17 to +18
k: compute the energy norm of the last snapshot, where the energy-product is constructed
with a randomly generated parameter instance with random seed integer k.

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.

Sorry, but why not just let the user specify the actual value? Specifying the seed seems weird, when it's only a single number we are talking about.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

You are absolutely right. I thought that problem 0 and 1 differ in terms of parameter space. I changed that now.

Comment thread src/pymordemos/elliptic2.py Outdated
Comment thread src/pymor/discretizers/builtin/cg.py Outdated
Tim Keil and others added 3 commits August 25, 2020 17:35
Co-authored-by: Stephan Rave <stephanrave@uni-muenster.de>
Co-authored-by: Stephan Rave <stephanrave@uni-muenster.de>
@sdrave sdrave merged commit 289e7dc into pymor:master Aug 25, 2020
@TiKeil TiKeil deleted the energy-product-in-elliptic-discretizer branch August 26, 2020 10:07
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.

4 participants