Design discussion: Support for keyword WCONHIST in sim_fibo_ad.cpp about opm-simulators HOT 34 CLOSED

hm, I have trouble understanding this issue: As far as I understood the Eclipse documentation, the main difference between WCONPROD and WCONHIST is that the rates for WCONPROD are upper limits while for WCONHIST they are the lower limit in addition (i.e. they must be matched by the well exactly). I have trouble to see where the problem with including a lower limit in the well equation is, at least if only a single component's rate must be matched. (and besides the fact that this approach to history matching smells like vodoo to me, because if one matches the rate, the BHP must be differ from the one specified and/or measured.)

from opm-simulators.

As far as I understood the Eclipse documentation, the main difference between WCONPROD and WCONHIST is that the rates for WCONPROD are upper limits while for WCONHIST they are the lower limit in addition

That's not it at all. Please read more carefully, especially the documentation on RESV control.

from opm-simulators.

@andlaus: With WCONHIST one typically has more data than needed to close the system of equations. For example one usually surface volume rates for all three components. The question is how to use that information. One could use a regular ORAT/LRAT/GRAT etc. control, but that could lead to very a-historic situations when for example water arrives at a well before (or after) the time it did in the history.

The RESV control appears to try to minimize abrupt changes to the control by making the extracted volume from the reservoir approximately the same as it was historically, without necessarily matching any of the three production rates given.

@bska: I do not understand why you need the topology etc. imported into the solver class. These things are available to it through the well struct. I think we should add a new element to the enum that designates the type of control though, perhaps RESERVOIR_RATE_DYNAMIC?

from opm-simulators.

I do not understand why you need the topology etc. imported into the solver class.

I don't. I want that in the examples/sim_fibo_ad.cpp file itself.

I think we should add a new element to the enum that designates the type of control though, perhaps RESERVOIR_RATE_DYNAMIC?

Why? Once the rate values have been translated to reservoir conditions it's a perfectly normal RESV target. I don't see the point of introducing a special case that all solvers have to adapt to when we already have the required facilities in place.

from opm-simulators.

First of all - very good to iniate such a discussion. I am not sufficiently familiar with the simulator codes to give really valuable contributions, but I have some thoughts which should at least be in the right part of the woods context wise.

The WellsManager in this case is class Opm::WellsManager from opm-core (file opm/core/wells/WellsManager.hpp). One of the objectives of class WellsManager is to turn the current notion of the present wells and associate constraints into the struct Wells that's used within the simulator and solver classes.

1. From the outside I really do not understand the need for the Wellsmanager or the struct Wells - as I see it all the relevant information is already in the EclipseState::Schedule class? I understand that the simulator should not be (too) Eclipse centric and dependant - but although the word Eclipse is not used explicitly in the WellsManagere / struct Wells combo I would argue that they are currently equally Eclipse centric.

2. If the purpose of the struct Wells is performance I would say that the structure (struct Wells or something similar) aimed at performance should be created and killed anonymously inside the simulator.

3. In principle roughly everything except the name of a well can change at any timestep, but the changes typically happen at two widely different time scales:

   • Changes in the rates typically happen for every timestep.
   • Changes in the toplogy, new wells added, injector <-> producer e.t.c. typically happen seldom,
     i.e. a couple of times for the complete simulation.

   The current implementation does not exploit this difference in times scales at all.

4. When discussing wells and so on I would like to draw the attention to this issue: #143

so short-term I think I'd like to duplicate the topology, well/completion index calculation and constraints portion of WellsManager::WellsManager() into examples/sim_fibo_ad.cpp and to introduce state-dependent constraints in that restricted environment.

Statements like that make my 'Copy-and-Paste' detector go ballistic. I think it is very dangerous and misleading to use the phrase:

so short-term I think I'd like to duplicate ... 

OK - the suggestion is to do some "C-c C-v" trickery to avoid dependency problems+++ Might very well be an acceptable, maybe even good(?), solution - but it is code duplication which inevitably leads to a maintainance problems, more bugs e.t.c. Maybe in the future someone will refactor this - maybe not? Sweetening the pill with so short-term seems to imply that removal of the duplication is automatically imminent - it is not.

For someone from the outside 'who was not there when it happened' the opm code already has quite a lot of such code duplication, and it certainly makes it more difficult to follow. So my quite unqualified gut feeling is that such code duplication is not worth it to support the RESV control mode.

Trying to finalize on a slightly more constructive mode: From different posts I have got the feeling that RESV control mode should be satisfied in a reasonably relaxed manner - that sounds very sensible to me.

from opm-simulators.

From the outside I really do not understand the need for the WellsManager or the struct Wells

Hysterical raisins. The struct Wells was inspired by an earlier project, before the existence of the opm-parser. At present all simulators that use wells are implemented in terms of struct Wells and struct WellControls* (with associate C-based API). Restructuring that code to use classes Opm::WellSet, Opm::Well, Opm::CompletionSet, and Opm::Completion from opm-parser while certainly straight forward is nevertheless a bit of work.

[All] the relevant information is already in the EclipseState::Schedule class?

The devil's in the details and I'm not sufficiently familiar with the opm-parser so I'll just ask: How do you calculate the Peaceman indices of each completion (or, where is this done)? In particular, what grid do you target? Is it exactly the same as that produced by opm-core's GridManager (or rather create_grid_cornerpoint()) in terms of cell volumes, number of active cells, cell topology &c?

If the purpose of the struct Wells is performance I would say that the structure (struct Wells or something similar) aimed at performance should be created and killed anonymously inside the simulator.

Not just performance. It is crucial that the completions (and associate Peaceman indices) match the simulation grid.

In principle roughly everything except the name of a well can change at any timestep, but the changes typically happen at two widely different time scales: [...] The current implementation does not exploit this difference in times scales at all.

Correct. We do not. That's one of the deficiencies of the current approach. We might even envision creating all wells ahead of the first time step and set up the matrix structures accordingly. I don't think we're able to do that right however because we're missing quite a few pieces in the simulator but it is one possible future development.

When discussing wells and so on I would like to draw the attention to this issue: #143

Yeah, that's clearly a bug related to changing wells. I haven't analysed the problem but @osae's original assessment that the direct cause is failure to adapt well solution quantities to changing number/topology of wells seems apt.

so short-term I think I'd like to duplicate [...]

Statements like that make my 'Copy-and-Paste' detector go ballistic. I think it is very dangerous

Agreed. If the Schedule really can provide all the required information then we should simply use it. That, in turn, hinges on the exact grid definition used to compute Peaceman indices.

from opm-simulators.

The devil's in the details and I'm not sufficiently familiar with the opm-parser so I'll just ask: How do you calculate the Peaceman indices of each completion (or, where is this done)? In particular, what grid do you target? Is it exactly the same as that produced by opm-core's GridManager (or rather create_grid_cornerpoint()) in terms of cell volumes, number of active cells, cell topology &c?

Well - that is slightly embarrassing, but currently the Schedule class does not accept defaulted connection factors, only values given explicitly in the deck are allowed. The reason was that at the time we did not have a good PERM? implementation, but that is very much in the coming - and then the plan was to employ the Peaceman formula based on the input grid.

Not just performance. It is crucial that the completions (and associate Peaceman indices) match the simulation grid.

That seems reasonable enough, but remember that in the very common case that the connection factors are given in the deck an implicit dependency to the input grid has already been enforced.

from opm-simulators.

How do you calculate the Peaceman indices of each completion

Well - that is slightly embarrassing, but currently the Schedule class does not accept defaulted connection factors, only values given explicitly in the deck are allowed.

Ah, I see.

[T]hat [calculation] is very much in the coming - and then the plan was to employ the Peaceman formula based on the input grid.

Right. If I recall correctly, the input grid is the one defined (purely) in terms of ACTNUM and MINPV (or MINPVV). Is that true? If so, then we will at least have to cater to the possibility that the simulation grid is slightly different, e.g., by deactivating cells due to pinch-outs.

from opm-simulators.

and then the plan was to employ the Peaceman formula based on the input grid.

Does that mean that you intend to put the Peaceman equation directly into opm-parser? If yes, I think that opm-parser is probably not the right module for this as IMHO its job is to express a c++ interface to deck files which is as convenient as possible; if you don't intend so, please ignore this comment and excuse my stupidity...

On a different note, I'm still confused about history matching wells: After re-reading the Eclipse documentation I currently believe that the main difference to regular wells is that for the history-matching flavor the observed well data can be written to the summary file (in addition to the ones calculated by the simulation). Is that view closer to correct than my previous one?

from opm-simulators.

the main difference to regular wells is that for the history-matching flavor the observed well data can be written to the summary file

Yes, well, maybe. The BIG issue in this particular case, however, is the interpretation of the RESV controls. The target value doesn't exist in the WCONHIST record. Rather, it is the simulator's responsibility to convert the observed component rates at surface conditions (ORAT, WRAT, and GRAT, items 4-6) to reservoir conditions and then sum the values. The surface-to-reservoir translation is defined in terms of average field conditions (mainly pressures) so the RESV target value changes from one timestep to the next. Essentially, we need complete field state and the fluid definition to compute those values.

That's the background of this Issue.

from opm-simulators.

Right. If I recall correctly, the input grid is the one defined (purely) in terms of ACTNUM and MINPV (or MINPVV). Is that true? If so, then we will at least have to cater to the possibility that the simulation grid is slightly different, e.g., by deactivating cells due to pinch-outs.

Well - the Schedule program from SLB, which I guess is the authority on the matter, uses an .EGRID file as input. So the grid employed in that case has been preprocessed with MINPV, PINCH .e.t.c. by the simulator.

However - I do not really see this question as a very important one, the deck is free to complete cells more or less all over the place, and the simulator must take the possibility of inactive cells into account when mapping the completed cells to the space of active cells used in the actual numerics - or??

from opm-simulators.

Does that mean that you intend to put the Peaceman equation directly into opm-parser? If yes, I think that opm-parser is probably not the right module for this as IMHO its job is to express a c++ interface to deck files which is as convenient as possible; if you don't intend so, please ignore this comment and excuse my stupidity...

Well - who is stupid is of course in the mind of the beholder. opm-parser is certainly a quite 'fat' parser, but as long as the raw underlying deck format is so weakly structured I see it as natural create aggregate objects at a higher level of abstraction than a pure list of deck keywords.

Right now the natural place for the Peaceman formula is in opm-core::Wellsmanager, where it to some extent already is. However - my evil plan is to get rid of all of WellsManager and then the Peacmen calculation would go in the opm-parser::Schedule object.

from opm-simulators.

@bska

The BIG issue in this particular case, however, is the interpretation of the RESV controls. The target value doesn't exist in the WCONHIST record. Rather, it is the simulator's responsibility to convert the observed component rates at surface conditions (ORAT, WRAT, and GRAT, items 4-6) to reservoir conditions and then sum the values.

I might be wrong again (wouln't be the first time by a long shot ;)), but I always thought that the regular wells also have a RESV control mode?

On a different note, RESV as defined by Eclipse makes approximately zero sense, because (if I'm not on the wrong track once more), they just sum the individual rates which means STB/day (liquids) plus MCF/day (gas) which equals garbage...

@joakim-hove

but as long as the raw underlying deck format is so weakly structured I see it as natural create aggregate objects at a higher level of abstraction than a pure list of deck keywords.

sure, I'm fully in favor for the parser to provide objects which express the dynamic behavior of the deck, but I think calculating the source terms would go too far into the realm of the simulator. For a start, it would need to interpret at the simulator-specific state of the reservoir to pull that feat... (and that looks quite different in e.g. eWoms vs. opm-autodiff.)

Right now the natural place for the Peaceman formula is in opm-core::Wellsmanager, where it to some extent already is. However - my evil plan is to get rid of all of WellsManager and then the Peacmen calculation would go in the opm-parser::Schedule object.

Refactoring the wells handling code is much appreciated, I just think that this should be done in the appropriate modules (e.g., opm-autodiff). If you are hell-bend on putting it into the schedule object, do you have a battle plan of how to properly abstract the reservoir's state?

from opm-simulators.

If you are hell-bend on putting it into the schedule object,

I'm not hell bent on anything; on the contrary I will bend as straw in the wind for good suggestions.

do you have a battle plan of how to properly abstract the reservoir's state?

Well - the only 'state' needed for this calculation is the permeability.

from opm-simulators.

On moving things into the Schedule class: the Wells struct is a C-compatible abstraction mechanism, and the WellsManager class provides (by reading from a Schedule object) a concrete instance for each timestep. If we want to move all of this into the Schedule class, it would be taking on the role of the Wells struct, and all well-usage would eventually go through it. This would mean replacing a C API with a C++ API (Wells struct -> Schedule class, or eventually an interface class of which Schedule would be a subclass), and we would lose the ability to use our existing (or future, although I am not planning any such) C codes with wells.

On the dynamic computation of the RESV control: I think it should be put in the FullyImplicitBlackoilSolver class, and that other classes in the chain of things (Schedule, Wells) should only serve to provide the control values as given in the deck (and extend them the tiny bit necessary).

from opm-simulators.

Well - the only 'state' needed for this calculation is the permeability.

I think you also need at least the phase pressures, and the relperms.

from opm-simulators.

@andlaus

I always thought that the regular wells also have a RESV control mode?

They certainly do. In the case of WCONPROD, however, the RESV target value is explicitly given in the deck. In WCONHIST the target value must be dynamically calculated. That's the difference.

[ECLIPSE's definition of RESV] just sum the individual rates which means STB/day (liquids) plus MCF/day (gas)

Not true. RESV is _RESERVOIR_ volume rates. At _RESERVOIR_ conditions. Those are all in STB/day (when using field units).

@joakim-hove

[I'd like] to get rid of all of WellsManager and then the Peaceman calculation would go in the opm-parser::Schedule object.

For the record: I'd like to see that happen too. As stated earlier, I think the utility of class Opm::WellsManager is dwindling rapidly. At present objects of that class are little more than std::unique_ptr<Wells> under another name (albeit with code in place to populate the Wells object). As for the parser presenting a list of keywords vs. a more fully defined state machine, well I guess I'm leaning towards the latter. I think it's hard to argue that all clients should take on the task of interpreting the default values in SWOF, for example.

@atgeirr

On the dynamic computation of the RESV control: I think it should be put in the FullyImplicitBlackoilSolver class

Actually, I think the SimulatorFullyImplicitBlackoil<Grid> is more natural. The *Solver<> deals in defined mathematical problems (including well and boundary constraints), whereas the Simulator*<> defines those problems (or should) for individual time steps.

from opm-simulators.

@atgeirr

we would lose the ability to use our existing (or future, although I am not planning any such) C codes with wells.

do you mean external C codes or OPM code? the latter would obviously need to be adapted during the refactoring while the former should not be a priority for this project IMHO.

On the dynamic computation of the RESV control: I think it should be put in the FullyImplicitBlackoilSolver class, and that other classes in the chain of things (Schedule, Wells) should only serve to provide the control values as given in the deck (and extend them the tiny bit necessary)

doesn't it need to go wherever the well rates are going to be calculated? Just wondering...

from opm-simulators.

Why? Once the rate values have been translated to reservoir conditions it's a perfectly normal RESV target. I don't see the point of introducing a special case that all solvers have to adapt to when we already have the required facilities in place.

I see that point: if a solver/assembly code already implements RESERVOIR_RATE targets we could accomplish this by simply setting the rate properly in the WellControls part of the Wells struct. But it did not answer my original question: what do you need the well information for in sim_fibo_ad.cpp?

If what you want to do is to create the Wells struct in sim_fibo_ad.cpp and deal with computing the RESV rate also there, I am not sure it is a good idea: it means that we are mixing code parts that could have been separate (Wells setup and the solver/state dependent things).

from opm-simulators.

@joakim-hove
the only 'state' needed for this calculation is the permeability.

@andlaus
I think you also need at least the phase pressures, and the relperms.

Depends on what "this" calculation is. If you're talking about connection factors (Peaceman indices), then we only need the permeability and the cell geometry (and direction of intersection). If you're talking about the WCONHIST RESV target value calculation, then we need a snapshot of the field conditions at the start of the time step (plus the fluid object). If you're talking about calculating connection fluxes you need more or less the complete dynamic, internal state of the simulator object.

@atgeirr

But it did not answer my original question: what do you need the well information for in sim_fibo_ad.cpp?

It's not so much that I need it in sim_fibo_ad.cpp, it's that I don't need the WellsManager. It gets in the way of what I want to do.

from opm-simulators.

What a wonderful discussion!

@joakim-hove
the only 'state' needed for this calculation is the permeability.

@andlaus
I think you also need at least the phase pressures, and the relperms.

Depends on what "this" calculation is. If you're talking about connection factors (Peaceman indices),

I was considering the connection factors.

from opm-simulators.

@bska

Not true. RESV is RESERVOIR volume rates. At RESERVOIR conditions. Those are all in STB/day (when using field units).

Another day, another inconsistency in Eclipse...

but that does not make it much better: since gas is highly compressible why would one ever be interested to control that rate? (I could understand the desire to control overall the surface rate, as there might be equipment with limited capacity...)

I think it's hard to argue that all clients should take on the task of interpreting the default values in SWOF, for example.

that's why there should continue to be an layer between the parser and the simulator which provides a higher level abstraction but makes design decisions (e.g. linear interpolation vs spline) which a given simulator may or may not follow.

Depends on what "this" calculation is. If you're talking about connection factors (Peaceman indices), then we only need the permeability and the cell geometry (and direction of intersection). If you're talking about the WCONHIST RESV target value calculation, then we need a snapshot of the field conditions at the start of the time step (plus the fluid object). If you're talking about calculating connection fluxes you need more or less the complete dynamic, internal state of the simulator object.

@joakim-hove answered

I was considering the connection factors.

Ah, okay. I thought joakim was talking about providing the source terms for each element as defined by the Peaceman model. Mental note to myself: Peaceman != Peaceman...

from opm-simulators.

But if I understood you correctly, you don't need it because you more or less duplicate its features in sim_fibo_ad.cpp? If that is you intention I would strongly recommend going the less invasive (and duplicating) route of adding a new member to the enum. That way only the solver class (or simulator class, depending on taste) and a (very) small bit in the WellsManager must change, and there is no duplication of code. The only drawback is that any other simulators that need this must also implement it, but that is already true, even if you want to re-use an existing implementation of RESERVOIR_RATE targets in the assembly code.

from opm-simulators.

Another day, another inconsistency in Eclipse...

but that does not make it much better: since gas is highly compressible why would one ever be interested to control that rate? (I could understand the desire to control overall the surface rate, as there might be equipment with limited capacity...)

RESV is _TOTAL_ reservoir volume rate, i.e.,

\sum_\alpha v_\alpha

for all (active) phases \alpha. At (typical) reservoir conditions the gas behaves in much the same way as a liquid, albeit with lower viscosity.

from opm-simulators.

But if I understood you correctly, you don't need [class WellsManager] because you more or less duplicate its features in sim_fibo_ad.cpp?

I think it's more or less the opposite. The WellsManager prevents me from doing what I want with dynamic constraints, so I need to extract the useful parts (well index calculation, setup of struct Wells) and then do the constraints myself.

from opm-simulators.

RESV is TOTAL reservoir volume rate

that's what I thought. I was just wondering why one would calculate the total reservoir volume instead of the surface volume...

from opm-simulators.

s/calculate/control/

from opm-simulators.

I would strongly recommend going the less invasive (and duplicating) route of adding a new member to the enum.

The more I think about it, the more convinced I become that that's poor design. I really don't want to do that.

from opm-simulators.

I was just wondering why one would control the total reservoir volume instead of the surface volume...

Mostly because it changes less rapidly with changing pressures than the component rates at surface conditions.

from opm-simulators.

I would strongly recommend going the less invasive (and duplicating) route of adding a new member to the enum.

The more I think about it, the more convinced I become that that's poor design.

Just to back up that point a bit more: As I already said, the WCONHIST/RESV is a standard RESV target once the target value is known. The solver wouldn't know what to do with it either, because the correct location to calculate the target value is before entering step(). Even if we did put the calculation within step(), the well equation would be like

switch (target type) {
case RESERVOIR_RATE:
    // Fall-through
case RESERVOIR_RATE_DYNAMIC:
    // assemble
    break;
}

so the RESERVOIR_RATE_DYNAMIC adds exactly no new insight.

from opm-simulators.

If what you want is a Wells struct that has the correct target value put into it dynamically, there is an easier way I think: in the simulator class (or near enough...) we compute the target and either

• simply assign it to the existing Wells struct's target member (requires us to make that a reference and not a const reference, and accept the consequences), or,
• create a duplicate of the Wells, assign dynamic targets in the duplicate, and pass it to the solver.

from opm-simulators.

If what you want is a struct Wells that has the correct target value put into it dynamically

That is, indeed, what I want. Of the two suggestions, the only workable solution in my opinion is the clone (typically managed by a std::something_ptr<Wells> and) created as

wells_(clone_wells(&W), &destoy_wells)

However, we must then go on to clear all controls in all wells and build them ourselves from a Schedule in each call to Impl::run(). That, essentially, replicates most of WellsManager::WellsManager() so I still don't see the point of using the WellsManager in the first place. It also means making the Simulator*::run() and Simulator*::Impl::run() methods aware of the Schedule and its lower-level structures.

from opm-simulators.

However, we must then go on to clear all controls in all wells and build them ourselves from a Schedule in each call to Impl::run(). That, essentially, replicates most of WellsManager::WellsManager() so I still don't see the point of using the WellsManager in the first place.

Since it is only the dynamic control that must be updated, that is all we need new code for: use the WellsManager for everything else, make a copy, then add the dynamic constraints to the copy. I don't see why we must clear all controls.

from opm-simulators.

I have used another approach in my support-resv branch. There are elements of the initial suggestion, but there is no wholesale code duplication. Thank you all for the inputs. I'm closing this issue.

from opm-simulators.