Skip to content

Linearization support for nonlinear domain decomposition methods#682

Merged
bska merged 1 commit into
OPM:masterfrom
atgeirr:aspin
Jun 8, 2023
Merged

Linearization support for nonlinear domain decomposition methods#682
bska merged 1 commit into
OPM:masterfrom
atgeirr:aspin

Conversation

@atgeirr

@atgeirr atgeirr commented Jan 10, 2022

Copy link
Copy Markdown
Member

Added to have a place for discussion, as for its downstream PR OPM/opm-simulators#3764.

@OPMUSER

OPMUSER commented Nov 2, 2022

Copy link
Copy Markdown

Can this be closed?

@bska

bska commented Nov 2, 2022

Copy link
Copy Markdown
Member

Can this be closed?

I don't think so. We should rather pick the work back up after the 2022.10 release.

@bska

bska commented Feb 6, 2023

Copy link
Copy Markdown
Member

This patch no longer applies cleanly. Would you mind rebasing the branch onto the current master sources please?

@atgeirr

atgeirr commented Feb 6, 2023

Copy link
Copy Markdown
Member Author

Will update.

@bska

bska commented May 26, 2023

Copy link
Copy Markdown
Member

Following #806 this now has merge conflicts. Please rebase.

@atgeirr atgeirr force-pushed the aspin branch 6 times, most recently from 517ff12 to f8c9b42 Compare June 6, 2023 09:53
@atgeirr atgeirr marked this pull request as ready for review June 6, 2023 09:54
@atgeirr

atgeirr commented Jun 6, 2023

Copy link
Copy Markdown
Member Author

jenkins build this please

@atgeirr atgeirr changed the title [WIP] Nonlinear domain decomposition methods Linearization support for nonlinear domain decomposition methods Jun 6, 2023

@bska bska 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 think this looks good apart from a few minor nits and clarifications.

Tangentially, I've noticed that a lot of the code in this module is written as

for (elemIt = ...) {
    const Element& elem = *elemIt;
    use(elem);
}

and that this PR follows that pattern too. Is there a reason, maybe lifetime extension, for creating the Element& instead of using *elemIt directly, especially if use() is just a single statement?

Comment thread opm/models/blackoil/blackoilnewtonmethod.hh Outdated
Comment thread opm/models/discretization/common/fvbasediscretization.hh Outdated
Comment thread opm/models/discretization/common/fvbaselinearizer.hh Outdated
Comment thread opm/models/discretization/common/fvbaselinearizer.hh Outdated
Comment thread opm/models/discretization/common/fvbaselinearizer.hh
Comment thread opm/models/discretization/common/tpfalinearizer.hh
@atgeirr

atgeirr commented Jun 7, 2023

Copy link
Copy Markdown
Member Author

Is there a reason, maybe lifetime extension, for creating the Element& instead of using *elemIt directly, especially if use() is just a single statement?

I think mostly readability, better to not deal directly with iterators. And then it has been duplicated everywhere needed. I agree that it is almost pointless when there is just a single usage though! There is one more argument in favour: less change needed to move to a range-for loop over view.elements(), which is something we should probably do anyway.

@atgeirr

atgeirr commented Jun 7, 2023

Copy link
Copy Markdown
Member Author

jenkins build this please

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

Thanks a lot for the updates. I think the code looks good now, and I'm prepared to merge the current end state. I noticed one potential refinement, but I'm not insisting that we write it that way. I would appreciate, however, if we could squash down the commit sequence to remove some of the intermediate "[HACK]" and "[WIP]" states.

Comment thread opm/models/blackoil/blackoilnewtonmethod.hh Outdated
This is intended to be used for nonlinear domain decomposition methods.
@atgeirr

atgeirr commented Jun 8, 2023

Copy link
Copy Markdown
Member Author

jenkins build this please

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

Thanks a lot for the updates and for squashing down the commit sequence. I'll merge this into the master branch now to enable bringing NLDD in too.

@bska bska merged commit 29a119a into OPM:master Jun 8, 2023
@atgeirr atgeirr deleted the aspin branch June 9, 2023 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants