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

Energy product in elliptic discretizer (Continuation) #1008

Merged
merged 24 commits into from
Aug 25, 2020

Conversation

TiKeil
Copy link

@TiKeil TiKeil commented Jul 9, 2020

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

TiKeil commented Jul 9, 2020

@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
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
Copy link
Member

sdrave commented Jul 16, 2020

@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
Copy link
Author

TiKeil commented Jul 16, 2020

Does anybody understand whats wrong with the tests again?

@renefritze
Copy link
Member

@TiKeil
Copy link
Author

TiKeil commented Jul 17, 2020

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
@TiKeil
Copy link
Author

TiKeil commented Jul 17, 2020

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

codecov bot commented Jul 17, 2020

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

TiKeil commented Jul 17, 2020

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

AUTHORS.md Outdated Show resolved Hide resolved
src/pymor/discretizers/builtin/cg.py Outdated Show resolved Hide resolved
src/pymor/discretizers/builtin/cg.py Outdated Show resolved Hide resolved
src/pymor/discretizers/builtin/cg.py Outdated Show resolved Hide resolved
src/pymor/discretizers/builtin/cg.py Outdated Show resolved Hide resolved
src/pymor/discretizers/builtin/cg.py Outdated Show resolved Hide resolved
src/pymordemos/elliptic2.py Outdated Show resolved Hide resolved
src/pymordemos/elliptic2.py Outdated Show resolved Hide resolved
@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
Copy link
Author

TiKeil commented Aug 3, 2020

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
src/pymor/discretizers/builtin/cg.py Outdated Show resolved Hide resolved
src/pymor/discretizers/builtin/cg.py Outdated Show resolved Hide resolved
src/pymordemos/elliptic2.py Outdated Show resolved Hide resolved
src/pymordemos/elliptic2.py Outdated Show resolved Hide resolved
src/pymordemos/elliptic2.py Outdated Show resolved Hide resolved
src/pymordemos/elliptic2.py Outdated Show resolved Hide resolved
@TiKeil TiKeil force-pushed the energy-product-in-elliptic-discretizer branch from f44847d to cf7ef1e Compare August 18, 2020 12:28
@TiKeil
Copy link
Author

TiKeil commented Aug 18, 2020

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 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
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
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.

src/pymordemos/elliptic2.py Outdated Show resolved Hide resolved
src/pymor/discretizers/builtin/cg.py Outdated Show resolved Hide resolved
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.

5 participants