Do not try to compute initial solution for inactive multi-segment wells split across processors #5751

vkip · 2024-11-19T14:58:51Z

No description provided.

vkip · 2024-11-19T14:59:01Z

jenkins build this please

bska · 2024-11-19T16:41:08Z

For what it's worth, this PR allows me to run a field case as mpirun -np 14 which, without this PR, crashes very early in the initialisation process.

I'll nevertheless defer to those more familiar with this part of the code to review the PR as there may be aspects of the structure that I don't fully grasp.

GitPaean · 2024-11-19T23:07:28Z

Is there some more information regarding the symptom? Where does it crash exactly?

bska · 2024-11-20T08:51:18Z

Where does it crash exactly?

In current master we crash in WellState<>::initWellStateMSWell() when indexing into an empty perf_rates object when n_activeperf > 0 (n_activeperf = 22 for w=0 in one of my test runs).

That said, if we want to use the proposed guard, then we should at least amend it to perf_rates.size() != n_activeperf * np, since there is supposed to be np entries for each active connection/perforation.

vkip · 2024-11-20T08:55:31Z

That said, if we want to use the proposed guard, then we should at least amend it to perf_rates.size() != n_activeperf * np, since there is supposed to be np entries for each active connection/perforation.

Or just perf_data.size() != n_active_perf, as it is currently in the PR?

GitPaean · 2024-11-20T13:31:02Z

My main concern is that with an if condition of inequality of these two variable is too broad for the targeted situation, might cover up other scenarios/bugs for future (we are not running distributed parallel ms wells yet, it should be addressed by that development for the situation of parallel ms well running).

If we know it was because that the well is SHUT, why do not we use that types of if condition to make it more clear that it was due to SHUT of the well. (at least something like ws.perf_data.size() == 0)

And also, let us output some DEBUG information or throw if ws.perf_data.size()) > 0, and ws.perf_data.size()) and n_activeperf are not equal. If it crashes in the future because they are not equal, then we check that specific scenario to have a more proper investigation and fixing.

vkip · 2024-11-20T14:00:01Z

My main concern is that with an if condition of inequality of these two variable is too broad for the targeted situation, might cover up other scenarios/bugs for future (we are not running distributed parallel ms wells yet, it should be addressed by that development for the situation of parallel ms well running).

I agree that the case with distributed active wells need to be handled by that development, hence the \todo message.

If we know it was because that the well is SHUT, why do not we use that types of if condition to make it more clear that it was due to SHUT of the well. (at least something like ws.perf_data.size() == 0)

When allowing to split inactive wells (that are never open at any time during the simulation) across processes, ws.perf_data.size() is not equal to zero here. There are perforations, since these wells may need to output RFT data, but each process may not have all of them.

Checking for SHUT sounds dangerous, since I guess wells may open during a time step..?

And also, let us output some DEBUG information or throw if ws.perf_data.size()) > 0, and ws.perf_data.size()) and n_activeperf are not equal. If it crashes in the future because they are not equal, then we check that specific scenario to have a more proper investigation and fixing.

Since this is not an error situation I think we should avoid DEBUG messages and definitely throws.

vkip · 2024-11-20T14:18:41Z

I can add a more explicit check for inactive wells, then (for now) throw for distributed wells. Does that sound ok?

… distributed multi-segment well

GitPaean · 2024-11-20T14:48:40Z

I can add a more explicit check for inactive wells, then (for now) throw for distributed wells. Does that sound ok?

Yes, that is sensible.

And we discussed a little bit. Since we decide some inactive wells can be distributed across processes, there should be a way/criteria to detect/decide which wells can be split. For those wells, since we can not do much (like opening them), let us do minimal things with them. For example, if possible, not initialize unneeded wellstate information (you are the one knows the best regarding this issue).

For the function initWellStateMSWell(), you can safely continue at the beginning of the for loop for those wells, and for init() and base_init(), we can also do less possibly but I am not familiar related to the RFT usage.

Please let us know what you think of it.

GitPaean · 2024-11-20T15:03:56Z

opm/simulators/wells/WellState.cpp

+            // \todo{ Update the procedure below to work for actually distributed wells. }
+            if (static_cast<int>(ws.perf_data.size()) != n_activeperf)
+                if (this->is_inactive_well(well_ecl.name()))
+                    continue;


Can we use the following code,

if (this->is_inactive_well(well_ecl.name())) continue;

right after the beginning of the for loop

for other scenarios, if (static_cast<int>(ws.perf_data.size()) != n_activeperf) we can throw while the new parallel ms wells development will need to handle it.

GitPaean · 2024-11-20T15:10:41Z

opm/simulators/wells/WellState.cpp

@@ -273,6 +273,7 @@ void WellState<Scalar>::init(const std::vector<Scalar>& cellPressures,
                                                                report_step,
                                                                wells_ecl);
    well_rates.clear();
+    this->inactive_well_names_ = schedule.getInactiveWellNamesAtEnd();


If it is using schedule.getInactiveWellNamesAtEnd(); to determine whether a well can be split across the processes, I will suggest a more specific name for inactive_well_names_, to show the wells will be shut all the time and can not be open across the simulation. Like permanently_inactive_well_names_ and the corresponding function name can be is_permanently_inactive_well.

vkip · 2024-11-20T15:26:41Z

jenkins build this please

GitPaean · 2024-11-20T21:04:40Z

@bska , can you test whether the current version fix the running of your case? I am happy with the current approach that has a more specific design to tackle the problem. You can review/merge as you will.

bska

can you test whether the current version fix the running of your case?

I've just completed a test of field case I mentioned before. I can confirm that the case continues to run in parallel (mpirun -np 14) with this edition of the PR. In the current master sources the case does not run in parallel, but it does run in sequential mode.

I am happy with the current approach that has a more specific design to tackle the problem.

It looks good to me too. At some point we may consider moving the Schedule::getInactiveWellNamesAtEnd() call to the WellState constructor, however. We call WellState<>::init() at least once for each report step and I don't really expect getInactiveWellNamesAtEnd() to change although I may be missing something.

In any case, this fixes a real problem on a real case so I'll merge into master.

lisajulia · 2024-11-22T07:49:11Z

can you test whether the current version fix the running of your case?

I've just completed a test of field case I mentioned before. I can confirm that the case continues to run in parallel (mpirun -np 14) with this edition of the PR. In the current master sources the case does not run in parallel, but it does run in sequential mode.

I am happy with the current approach that has a more specific design to tackle the problem.

It looks good to me too. At some point we may consider moving the Schedule::getInactiveWellNamesAtEnd() call to the WellState constructor, however. We call WellState<>::init() at least once for each report step and I don't really expect getInactiveWellNamesAtEnd() to change although I may be missing something.

In any case, this fixes a real problem on a real case so I'll merge into master.

@bska can you rerun the test field case you were running with mpirun -np 14 once more with the current master and/or send me the file so I can also check this on my side?
I've been working on running MSWells in parallel, I've split my work into two PRs (assembly #5680 and solving #5746) and I would like to test with that file as well.

Thanks!

bska · 2024-11-22T08:07:17Z

can you rerun the test field case you were running with mpirun -np 14 once more with the current master

Sure. Is there anything in particular you'd like me to look out for?

lisajulia · 2024-11-22T08:24:34Z

can you rerun the test field case you were running with mpirun -np 14 once more with the current master

Sure. Is there anything in particular you'd like me to look out for?

Nothing in particular, just check if the case runs through as expected. Thanks!

bska · 2024-11-22T08:30:38Z

can you rerun the test field case you were running with mpirun -np 14 once more with the current master

Sure. Is there anything in particular you'd like me to look out for?

Nothing in particular, just check if the case runs through as expected

Cool. I'll just rebuild everything first to make sure I have a consistent set of binaries given the CMake changes that were just merged.

bska · 2024-11-22T09:43:16Z

Nothing in particular, just check if the case runs through as expected

@lisajulia : The model does indeed still run as mpirun -np 14.

GitPaean · 2024-11-22T09:49:00Z

I think the concern only applies when we actually distribute the MS wells across processes.

lisajulia · 2024-11-22T09:56:33Z

Yes :) @bska : can you also try with this PR? #5746

bska · 2024-11-22T11:28:14Z

can you also try with PR #5746?

I got slightly different timestepping behaviour between master and that PR, but not different enough that it's possible to say that one run is "better" than the other. Final TCPU is currently slightly higher with #5746 than in master as of #5756.

On a sidenote, if AllowDistributedWells is supposed to work as of #5746, then there's still something missing as I get the diagnostic below when setting the value to true.

Error: Option --allow-distributed-wells=true is only allowed if model
only has only standard wells. You need to provide option 
 with --enable-multisegement-wells=false to treat existing 
multisegment wells as standard wells.

Error: [${ROOT}/opm-simulators/opm/simulators/flow/FlowGenericVanguard.cpp:332] All wells need to be standard wells!

lisajulia · 2024-11-22T11:47:30Z

Ok thanks, I will take this setting into account for my PR #5746 !

akva2 · 2024-11-22T12:00:16Z

do address the typo in the message as well (--enable-multisegment-wells=false)

lisajulia · 2024-11-22T14:18:15Z

do address the typo in the message as well (--enable-multisegment-wells=false)

6bdb801#diff-cdbb36d3d28bb6896b6aa7d316bc42496e4feb0bca83f210919e4826dc7f275dR327

Do not try to compute initial solution for inactive multi-segment wells

c42f02e

Explicit check for inactive wells and throw if trying to initialize a…

e189d56

… distributed multi-segment well

GitPaean reviewed Nov 20, 2024

View reviewed changes

Earlier continue and rename vector

673d541

vkip force-pushed the dont_initsolve_inactive_msw branch from b60eecb to 673d541 Compare November 20, 2024 15:25

bska approved these changes Nov 21, 2024

View reviewed changes

bska merged commit 20a13ee into OPM:master Nov 21, 2024
1 check passed

vkip deleted the dont_initsolve_inactive_msw branch November 21, 2024 09:20

GitPaean mentioned this pull request Nov 21, 2024

Feature/ms wells - part 1: Initial assembly of B C D and the residual #5680

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not try to compute initial solution for inactive multi-segment wells split across processors #5751

Do not try to compute initial solution for inactive multi-segment wells split across processors #5751

vkip commented Nov 19, 2024

vkip commented Nov 19, 2024

bska commented Nov 19, 2024

GitPaean commented Nov 19, 2024

bska commented Nov 20, 2024

vkip commented Nov 20, 2024

GitPaean commented Nov 20, 2024 •

edited

Loading

vkip commented Nov 20, 2024

vkip commented Nov 20, 2024

GitPaean commented Nov 20, 2024 •

edited

Loading

GitPaean Nov 20, 2024

GitPaean Nov 20, 2024

vkip commented Nov 20, 2024

GitPaean commented Nov 20, 2024

bska left a comment

lisajulia commented Nov 22, 2024 •

edited

Loading

bska commented Nov 22, 2024

lisajulia commented Nov 22, 2024

bska commented Nov 22, 2024

bska commented Nov 22, 2024

GitPaean commented Nov 22, 2024 •

edited

Loading

lisajulia commented Nov 22, 2024 •

edited

Loading

bska commented Nov 22, 2024

lisajulia commented Nov 22, 2024

akva2 commented Nov 22, 2024

lisajulia commented Nov 22, 2024

Do not try to compute initial solution for inactive multi-segment wells split across processors #5751

Do not try to compute initial solution for inactive multi-segment wells split across processors #5751

Conversation

vkip commented Nov 19, 2024

vkip commented Nov 19, 2024

bska commented Nov 19, 2024

GitPaean commented Nov 19, 2024

bska commented Nov 20, 2024

vkip commented Nov 20, 2024

GitPaean commented Nov 20, 2024 • edited Loading

vkip commented Nov 20, 2024

vkip commented Nov 20, 2024

GitPaean commented Nov 20, 2024 • edited Loading

GitPaean Nov 20, 2024

Choose a reason for hiding this comment

GitPaean Nov 20, 2024

Choose a reason for hiding this comment

vkip commented Nov 20, 2024

GitPaean commented Nov 20, 2024

bska left a comment

Choose a reason for hiding this comment

lisajulia commented Nov 22, 2024 • edited Loading

bska commented Nov 22, 2024

lisajulia commented Nov 22, 2024

bska commented Nov 22, 2024

bska commented Nov 22, 2024

GitPaean commented Nov 22, 2024 • edited Loading

lisajulia commented Nov 22, 2024 • edited Loading

bska commented Nov 22, 2024

lisajulia commented Nov 22, 2024

akva2 commented Nov 22, 2024

lisajulia commented Nov 22, 2024

GitPaean commented Nov 20, 2024 •

edited

Loading

GitPaean commented Nov 20, 2024 •

edited

Loading

lisajulia commented Nov 22, 2024 •

edited

Loading

GitPaean commented Nov 22, 2024 •

edited

Loading

lisajulia commented Nov 22, 2024 •

edited

Loading