changed: only keep keywords in sched_deck if there are actions

[akva2]

This becomes very significant with large schedules. This is probably too naive but I want to check fallout.

[akva2]

jenkins build this please

[akva2]

jenkins build this please

[akva2]

jenkins build this opm-simulators=5635 please

[bska]

I haven't looked at the details here yet, I'll do that later, but I just want to point out that in a restarted simulation run (rst != nullptr) it is possible, though unlikely, that all of the ACTIONX data has been entered prior to the restart step. If we're going with this approach then you might want to add something like

if (!keepKeywords) {
    keepKeywords = !rst->actions.empty();
}

to the if (rst) branch. This includes RstState::actions as one more source of information on whether to keep the input keywords.

[akva2]

done.

[akva2]

jenkins build this opm-simulators=5635 please

[akva2]

jenkins build this opm-simulators=5635 please

[blattms]

I have no overview here, so my question might make no sense.

How does this behave with MULT* in the schedule? We do have tests for this (opm-tests/mult/MULT2D_SCHD?.DATA), but even there the changes are still at the start of the simulation.

[akva2]

It would not affect it. The normal way the schedule operate is that we

1. list/copy all the keywords into the ScheduleDeck class.
2. iterate over the schedule deck and parse the keywords into appropriate classes (well instances with new multipliers applied) for each step. this is done once, before the simulation starts.

Then we can throw away the keywords as everything has been stored in those classes. The exception for this is if there is an action. It will then, once an action triggers, replay the deck keywords starting with the step the action happened.

Thus we only need these keywords IF there are actions that need the replaying when triggered.

[bska]

Does the API change here mean that this must be merged together with the companion PR (i.e., OPM/opm-simulators#5635)?

[bska]

This looks good to me and should be merged. I'll defer merging, however, until I know if the downstream PR must be merged alongside this PR.

[akva2]

There is an api change, but as there is a default value corresponding to old behavior it should in theory build separate of the downstream. I did not test that however.

[bska]

there is a default value corresponding to old behavior it should in theory build separate of the downstream

I saw the new default value, but I'm always uneasy when adding new defaulted parameters in the middle of the parameter list.

I did not test that however

Okay, I'll run a build check now to see.

[bska]

jenkins build this please

[bska]

I did not test [if this PR could be merged in isolation] however

Okay, I'll run a build check now to see.

That experiment failed so the two PRs must be merged in concert. Very well, I'll have a look at the other PR as well.

[akva2]

yeah, i can't resolve that one as it passed values for parameters after the added parameter.

[bska]

I've reviewed the downstream PR and everything looks good. I'm open to discussing what the default value of keepKeywords should be at the simulator level, but that's a separate issue to having the feature. I'll merge this and its downstream companion PR into the master branch now.