Skip to content

Add outputs to FV discretizers#1196

Merged
ftalbrecht merged 9 commits into
mainfrom
add-outputs-to-fv
May 26, 2021
Merged

Add outputs to FV discretizers#1196
ftalbrecht merged 9 commits into
mainfrom
add-outputs-to-fv

Conversation

@ftalbrecht

Copy link
Copy Markdown
Contributor

No description provided.

@ftalbrecht ftalbrecht added the pr:new-feature Introduces a new feature label Dec 1, 2020
@ftalbrecht ftalbrecht requested a review from sdrave December 1, 2020 10:08
@codecov

codecov Bot commented Dec 1, 2020

Copy link
Copy Markdown

Codecov Report

Merging #1196 (0c1fce3) into main (0a076cd) will decrease coverage by 0.01%.
The diff coverage is 72.58%.

Impacted Files Coverage Δ
src/pymor/discretizers/builtin/fv.py 67.36% <71.66%> (+0.50%) ⬆️
src/pymor/operators/constructions.py 89.10% <100.00%> (+0.01%) ⬆️
src/pymortests/demos.py 90.12% <0.00%> (-1.24%) ⬇️
src/pymortests/vectorarray.py 92.02% <0.00%> (+0.11%) ⬆️
src/pymor/discretizers/builtin/cg.py 92.07% <0.00%> (+0.17%) ⬆️
src/pymor/vectorarrays/numpy.py 85.20% <0.00%> (+0.23%) ⬆️
src/pymor/bindings/ngsolve.py 79.05% <0.00%> (+0.67%) ⬆️
src/pymor/tools/deprecated.py 90.90% <0.00%> (+4.54%) ⬆️

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

LGTM. But there is still some test missing that uses the code, right?

@ftalbrecht

Copy link
Copy Markdown
Contributor Author

LGTM. But there is still some test missing that uses the code, right?

Yes. We could adapt a demo to include a time-depdent boundary output term and also plot a relative output error in the reduction_error_analysis (which would also resolve @renefritze request in #1179). However, we would need a MOR + FV demo, and the only one I'm seeing there is burgers_ei, which does not (yet) include the error analysis.

Suggestions, @sdrave?

@sdrave

sdrave commented Dec 4, 2020

Copy link
Copy Markdown
Member

Suggestions, @sdrave?

Maybe show the elliptic demo some love and and compute some output quantities? No need for a time-dependent problem, I'd say.

@sdrave sdrave added this to the 2021.1 milestone Dec 4, 2020
@ftalbrecht

Copy link
Copy Markdown
Contributor Author

Suggestions, @sdrave?

Maybe show the elliptic demo some love and and compute some output quantities? No need for a time-dependent problem, I'd say.

I would not call it love, but I added unconditional computation of both kind of outputs to the demo.

@sdrave sdrave changed the base branch from master to main January 16, 2021 22:26
@sdrave

sdrave commented Mar 17, 2021

Copy link
Copy Markdown
Member

@ftalbrecht, shall we merge this PR before it goes stale?

@ftalbrecht

Copy link
Copy Markdown
Contributor Author

No objections from my side! However, I do not have any time at all at the moment (at least until Easter), for reasons.

@ftalbrecht ftalbrecht force-pushed the add-outputs-to-fv branch from e6367c3 to 38bd2a0 Compare May 25, 2021 21:11
@ftalbrecht ftalbrecht enabled auto-merge May 25, 2021 21:12
@renefritze renefritze force-pushed the add-outputs-to-fv branch from fd555db to 0c1fce3 Compare May 26, 2021 10:12
@ftalbrecht ftalbrecht merged commit 44479cb into main May 26, 2021
@ftalbrecht ftalbrecht deleted the add-outputs-to-fv branch May 26, 2021 11:32
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.

2 participants