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.

Sign up for GitHub

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

Jump to bottom

Added two-phase executable for numEq = 2. #1235

Merged
merged 1 commit into from Jun 29, 2017
Merged

Added two-phase executable for numEq = 2. #1235

merged 1 commit into from Jun 29, 2017

Conversation

dr-robertk
Copy link
Member

This PR adds an executable for two-phases cases with hard coded numEq = 2. This results in faster execution times for theses cases. This PR needs OPM/opm-models#204.

@atgeirr
Copy link
Member

atgeirr commented Jun 27, 2017

That is great! What kind of speedup do you see?

Eventually we want to have this included in a single flow executable (along with solvent, polymer etc.), but this would be the important step, I expect it to be fairly easy (if not trivial) to instantiate a model at run-time depending on input. (Might get a fairly big executable though.)

@dr-robertk
Copy link
Member Author

@atgeirr: Speedup is roughly 2.

@atgeirr
Copy link
Member

atgeirr commented Jun 28, 2017

I'll review this before the end of the week, I hope that is quick enough?

@dr-robertk dr-robertk force-pushed the PR/bo-two-phase branch 2 times, most recently from 60bca0b to 794c180 Compare June 28, 2017 13:40
@andlaus
Copy link
Contributor

andlaus commented Jun 28, 2017

jenkins build this please

@andlaus
Copy link
Contributor

andlaus commented Jun 28, 2017

the ewoms part has just been merged.

@dr-robertk
Copy link
Member Author

jenkins build this please

@dr-robertk
Copy link
Member Author

@atgeirr: This test fails because the data for flow_ebos_2p does not exist yet in opm-data. This could be a simple link to flow_ebos. Or should we copy the data?

@atgeirr
Copy link
Member

atgeirr commented Jun 29, 2017

Or should we copy the data?

I think it should have a separate reference, after all we do not expect it to produce bitwise identical results. They should be close however, so taking a copy (at least initially) seems reasonable.

@dr-robertk
Copy link
Member Author

So then I copy and make a PR in opm-data.

@dr-robertk
Copy link
Member Author

jenkins build this please with opm-data=#233

@dr-robertk
Copy link
Member Author

Is there are jenkins cmd list?

@dr-robertk
Copy link
Member Author

jenkins build this with opm-data=233 please

@blattms
Copy link
Member

blattms commented Jun 29, 2017 via email

@dr-robertk
Copy link
Member Author

jenkins build this please

atgeirr
atgeirr reviewed Jun 29, 2017
View reviewed changes
Copy link
Member

@atgeirr atgeirr left a comment

Choose a reason for hiding this comment

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

Looks very nice! Only two very minor nitpicks. Currently testing with a significant benchmark case.

CMakeLists_files.cmake
@@ -113,6 +113,7 @@ list (APPEND EXAMPLE_SOURCE_FILES
examples/flow_reorder.cpp
examples/flow_sequential.cpp
examples/flow_ebos.cpp
examples/flow_ebos_2p.cpp
Copy link
Member

Choose a reason for hiding this comment

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

I suggest to add it to the next list below also, of programs that will be installed.

examples/flow_ebos_2p.cpp
NEW_TYPE_TAG(EclFlowTwoPhaseProblem, INHERITS_FROM(EclFlowProblem));
//! The indices required by the model
SET_TYPE_PROP(EclFlowTwoPhaseProblem, Indices,
Ewoms::BlackOilTwoPhaseIndices<GET_PROP_VALUE(TypeTag, EnableSolvent)?1:0, GET_PROP_VALUE(TypeTag, EnablePolymer)?1:0, /*PVOffset=*/0>);
Copy link
Member

Choose a reason for hiding this comment

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

I see that this is intended to work with solvent and polymer, good! Has it been tested?

opm/autodiff/StandardWellsDense.hpp
static const int numEq = BlackoilIndices::numEq;
static const int numWellEq = GET_PROP_VALUE(TypeTag, EnablePolymer)? 3:numEq; // //numEq; //number of wellEq is only for 3 for polymer
static const int numWellEq = GET_PROP_VALUE(TypeTag, EnablePolymer)? numEq-1 : numEq; // //numEq; //number of wellEq is only numEq for polymer
Copy link
Member

Choose a reason for hiding this comment

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

I think this fix is correct, but the comment is wrong, should be (numEq - 1) I think.

opm/autodiff/StandardWellsDense_impl.hpp
@@ -239,7 +239,7 @@ namespace Opm {
}

// add trivial equation for 2p cases (Only support water + oil)
Copy link
Member

Choose a reason for hiding this comment

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

Just curious: I assume there is some kind of checking that we do not try to run this with (oil + gas) or (water + gas)?

@atgeirr
Copy link
Member

atgeirr commented Jun 29, 2017

It looks like you don't have Jenkins trigger privileges here. That is a bug, but I don't know how to fix it (and probably do not have the privilege to change the privileges...) so I'll trigger it.

@atgeirr
Copy link
Member

atgeirr commented Jun 29, 2017

jenkins build this please

@atgeirr
Copy link
Member

atgeirr commented Jun 29, 2017

I have now run a benchmark case, and timing results are as follows:

flow_legacy   2017.04   1151.33
flow_ebos     2017.04   1302.24
flow_ebos     current   783.057
flow_ebos_2p  current   455.408

Great job!

It should be noted that most of the improvement seen in flow_ebos comes from the changed tuning: only taking 10 nonlinear iterations before declaring failure, and ignoring cell-wise residuals after 8 iterations. This causes the curves for this case to change, probably due to less timestep cutting and therefore longer timesteps in some parts of the schedule. But the curves for flow_ebos and flow_ebos_2p are almost identical, which means this PR accomplishes its aim.

@atgeirr
Copy link
Member

atgeirr commented Jun 29, 2017

To continue the above: limiting the step size to something reasonable in the above case makes flow_legacy, flow_ebos before tuning and flow_ebos_2p give very close results. Timings with this limitation:

flow_legacy   2017.04   1480.91
flow_ebos     2017.04   1626.34
flow_ebos_2p  current    582.14

@atgeirr atgeirr merged commit f0b60cf into OPM:master Jun 29, 2017
@dr-robertk
Copy link
Member Author

Thanks.

@atgeirr: The current version only works with water+oil, because for the other cases not tests exist. So it does not make sense to start investigating these cases right now.

@dr-robertk dr-robertk deleted the PR/bo-two-phase branch June 29, 2017 13:03
@atgeirr
Copy link
Member

atgeirr commented Jun 29, 2017

So it does not make sense to start investigating these cases right now.

I agree, I just hope that any user who tries gets an understandable error message...!

@dr-robertk
Copy link
Member Author

@atgeirr: For the two-phase cases it sometimes makes sense to use
linear_solver_use_amg=true
At least for the cases I have tested that was faster.

@atgeirr
Copy link
Member

atgeirr commented Jun 29, 2017

For the current case (with the limited timesteps) I got 404.359 seconds, so it definitely helps!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atgeirr atgeirr atgeirr left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@dr-robertk @atgeirr @andlaus @blattms