Skip to content

Add GPTABLE (and GPTDIMS) gas plant table support to the Schedule#5214

Open
arturcastiel wants to merge 4 commits into
OPM:masterfrom
arturcastiel:gptable-schedule-consumer
Open

Add GPTABLE (and GPTDIMS) gas plant table support to the Schedule#5214
arturcastiel wants to merge 4 commits into
OPM:masterfrom
arturcastiel:gptable-schedule-consumer

Conversation

@arturcastiel

Copy link
Copy Markdown
Member

Add GPTABLE (and GPTDIMS) gas plant table support to the Schedule

What

Adds the GPTABLE keyword — gas plant recovery tables — to opm-common, plus the GPTDIMS RUNSPEC dimension keyword. Parse and represent only; no gas-plant recovery physics.

GPTABLE is valid in both SOLUTION and SCHEDULE and is a numbered, time-indexed table, so it follows the VFPPROD pattern: it is stored as ScheduleState::map_member<int, GptableTable>, keyed by the gas plant table number. The SOLUTION occurrence seeds report step 0 (the same mechanism VAPPARS uses via ScheduleStatic), and SCHEDULE occurrences update later steps (copy-on-write per report step).

GPTDIMS is registered as a parsed-but-ignored RUNSPEC dimension keyword — like VFPPDIMS/VFPIDIMS, the dynamic container needs no declared sizing, but real decks must still parse cleanly.

Details

  • Schedule/GptableTable.{hpp,cpp} — one table per GPTABLE record. Reads the gas plant table number, the lower/upper heavy-component indices (defaulting to the component count), and the recovery table (one heavy mole fraction followed by one recovery fraction per component, i.e. Ncomps + 1 values per row). Validates that the table number is provided and positive, that each component index lies in [1, Ncomps], and that the data length is a positive multiple of Ncomps + 1. The component-index ordering is intentionally not constrained, matching the reference simulator's behaviour. name(), operator==, serializeOp, and serializationTestObject mirror VFPProdTable.
  • ScheduleState — adds map_member<int, GptableTable> gptable, wired into serializeOp, operator==, and serializationTestObject.
  • KeywordHandlershandleGPTABLE builds one table per record and emits a GPTABLE_UPDATE schedule event; the component count comes from Runspec::numComps() (COMPS).
  • ScheduleStatic + Schedule::create_first — the SOLUTION-section GPTABLE is read into gptable_solution and seeded at report step 0, mirroring the VAPPARS path.
  • Keyword definitions — new JSON for GPTABLE (valid in SOLUTION+SCHEDULE; leading INT items followed by a trailing all-data DOUBLE) and GPTDIMS (RUNSPEC; parsed-but-ignored).
  • Tests — parser tests for the SOLUTION-seed + SCHEDULE re-specification (copy-on-write), defaulted indices resolving to the component count, and the validation guards; a serialization round-trip; and a combined test where a FIELDSEP stage references a GPTABLE (the two keywords are one feature — a separator stage may use a gas plant table instead of an EoS flash).

Scope

Base GPTABLE only — GPTABLEN / GPTABLE3 are out of scope.

@arturcastiel arturcastiel added the manual:irrelevant This PR is a minor fix and should not appear in the manual label Jun 9, 2026
@arturcastiel

Copy link
Copy Markdown
Member Author

jenkins build this please

@GitPaean GitPaean added manual:new-feature This is a new feature and should be described in the manual and removed manual:irrelevant This PR is a minor fix and should not appear in the manual labels Jun 9, 2026
@arturcastiel arturcastiel added manual:irrelevant This PR is a minor fix and should not appear in the manual and removed manual:new-feature This is a new feature and should be described in the manual labels Jun 9, 2026
@GitPaean

GitPaean commented Jun 9, 2026

Copy link
Copy Markdown
Member

It is a new keyword, why you decided/insisted it to be a manual:irrelevant?

@arturcastiel

Copy link
Copy Markdown
Member Author

It is a new keyword, why you decided/insisted it to be a manual:irrelevant?

sorry, I thought I had marked this. I did not see that you changed it =D. For me it is still irrelevant because the keywords is not usable yet.

@arturcastiel arturcastiel added manual:new-feature This is a new feature and should be described in the manual and removed manual:irrelevant This PR is a minor fix and should not appear in the manual labels Jun 9, 2026
@arturcastiel arturcastiel marked this pull request as ready for review June 9, 2026 18:25
@GitPaean

Copy link
Copy Markdown
Member

Thanks for the work. We will review it as soon as possible.

@GitPaean

Copy link
Copy Markdown
Member

Please rebase the branch, thanks.

@arturcastiel arturcastiel force-pushed the gptable-schedule-consumer branch from c61c729 to 9afa94d Compare June 16, 2026 14:08
@arturcastiel

Copy link
Copy Markdown
Member Author

jenkins build this please

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds support for parsing and storing Eclipse300 gas-plant recovery tables (GPTABLE) in the schedule state (including SOLUTION seeding at report step 0) and registers the GPTDIMS RUNSPEC dimension keyword so compositional decks containing these keywords parse cleanly.

Changes:

  • Introduce Opm::GptableTable to represent a single gas-plant recovery table record, including validation and serialization support.
  • Wire GPTABLE into schedule parsing/state: SOLUTION-section tables are cached in ScheduleStatic and seeded into report step 0; SCHEDULE-section updates are handled via a new keyword handler and a new schedule event.
  • Add keyword definitions (GPTABLE, GPTDIMS) and tests covering seeding, respec/copy-on-write, defaulted indices, validation failures, and serialization.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated no comments.

Show a summary per file
File Description
tests/test_Serialization.cpp Adds GptableTable to the serialization test matrix.
tests/parser/ScheduleTests.cpp Adds parser tests for SOLUTION seeding, SCHEDULE updates, defaults, validation guards, and FIELDSEP cross-reference usage.
opm/input/eclipse/share/keywords/keyword_list.cmake Registers new Eclipse300 keywords GPTABLE and GPTDIMS.
opm/input/eclipse/share/keywords/001_Eclipse300/G/GPTDIMS Defines GPTDIMS as a RUNSPEC dimension keyword (parsed for deck compatibility).
opm/input/eclipse/share/keywords/001_Eclipse300/G/GPTABLE Defines GPTABLE for SOLUTION+SCHEDULE parsing (table number + component bracket + trailing data).
opm/input/eclipse/Schedule/ScheduleStatic.hpp Adds storage for SOLUTION-section GPTABLE tables (gptable_solution) and serializes it.
opm/input/eclipse/Schedule/ScheduleStatic.cpp Parses SOLUTION-section GPTABLE occurrences into ScheduleStatic::gptable_solution.
opm/input/eclipse/Schedule/ScheduleState.hpp Adds ScheduleState::gptable map container and serializes it.
opm/input/eclipse/Schedule/ScheduleState.cpp Adds equality/serialization test object coverage for gptable.
opm/input/eclipse/Schedule/Schedule.cpp Seeds SOLUTION GPTABLE tables into report step 0 during create_first().
opm/input/eclipse/Schedule/KeywordHandlers.cpp Implements and registers handleGPTABLE() to update per-step schedule state and emit an event.
opm/input/eclipse/Schedule/GptableTable.hpp New table representation API for a single GPTABLE record, including serialization and equality.
opm/input/eclipse/Schedule/GptableTable.cpp Implements parsing and validation for GptableTable from a DeckRecord.
opm/input/eclipse/Schedule/Events.hpp Adds ScheduleEvents::GPTABLE_UPDATE.
CMakeLists_files.cmake Adds new source/header to the build.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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

It is a keyword I have not thought through the usage of it yest. So the comments are shallow and I think they are valid. Please address them.

As it said, there will be more comments coming.

int m_upper_component{};
int m_num_components{};
std::vector<double> m_data{};
KeywordLocation m_location{};

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.

What is the reason that m_location {} is a member variable?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It mirrors VFPProdTable, which GptableTable shares the per-step ScheduleState container with (map_member<int, …>, keyed by table number). VFPProdTable likewise stores a KeywordLocation m_location, exposes location(), and serializes it, so the location travels with the table and a located diagnostic is available once the recovery data has a consumer.

I kept it for parity with that pattern, but I'm happy to drop it (and from operator==/serializeOp) until a consumer actually needs it — your call.

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.

yes, we should drop it for now. We can add it back when we have code that needs it, otherwise, it is easy to have unused members hanging there.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done — dropped it. Removed the m_location member, the location() accessor, and its
terms in operator== and serializeOp. Agreed it shouldn't ride along until a consumer
needs located diagnostics; it's quick to reinstate then.

@@ -0,0 +1,113 @@
/*
Copyright 2024 Equinor ASA.

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 it should be 2026.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed — header year updated to 2026.

@@ -0,0 +1,127 @@
/*
Copyright 2024 Equinor ASA.

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.

same here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fixed — same here, updated to 2026.

{
using ParserKeywords::GPTABLE;

if (numComponents < 1) {

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.

You can use assert(numComponents >= 1); here. COMPS is not part of the GPTABLE keyword input.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Following up from the new batch: per your KeywordHandlers/ScheduleStatic comments, the
parse-site check is now runspec.compositionalMode() rather than a numComps() value
guard, so this is settled in that direction — see those threads.

}

const auto& table_num_item = record.getItem<GPTABLE::GAS_PLANT_TABLE_NUM>();
if (! table_num_item.hasValue(0)) {

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.

better to use if (table_num_item.defaultApplied(0)) {.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done — now uses table_num_item.defaultApplied(0). Item 1 has no parser default,
so a defaulted position is the empty default and defaultApplied(0) is true exactly
when the table number was omitted — the case the message describes.

// Items 2 and 3 default to the last component (numComponents) when omitted.
const auto& lower_item = record.getItem<GPTABLE::LOWER_COMPONENT>();
const auto& upper_item = record.getItem<GPTABLE::UPPER_COMPONENT>();
this->m_lower_component = lower_item.hasValue(0) ? lower_item.get<int>(0) : numComponents;

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.

Here also better to be use defaultApplied(0).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done — folded into the same idiom for the lower/upper indices:
defaultApplied(0) ? numComponents : get<int>(0).

};
}

this->m_table_num = table_num_item.get<int>(0);

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 would like to get a valid parsed table_num first, before assigning it to this->m_table_num.

Same for the m_lower_component and m_upper_component below.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done — the table number, the heavy-component indices, and the data are parsed into
locals and validated first; the members are assigned only once everything checks out.

};
}

this->m_data = record.getItem<GPTABLE::DATA>().getData<double>();

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.

m_data same. You should make sure the data is valid before assigning it to this->m_data.

then you can

this->m_data = std::move(data);.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done — m_data is validated as a local first, then this->m_data = std::move(data);.

throw OpmInputError {
fmt::format("GPTABLE table {}: data length ({}) must be a positive "
"multiple of the number of components + 1 ({}).",
this->m_table_num, this->m_data.size(), row_width),

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.

This only validates the shape of the table. There are much more criteria should be be satisfied if want to have a valid table.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

You answered the two questions I had left open here in the new batch, so I have implemented
both: every value is bounded to [0, 1] (the recovery values are normalized fractions) and
the key column is required strictly increasing. See the cpp:105 and cpp:109 threads.

Comment thread opm/input/eclipse/Schedule/Events.hpp Outdated
REQUEST_OPEN_COMPLETION = (UINT64_C(1) << 24),

/// A GPTABLE gas plant table has been specified or updated
GPTABLE_UPDATE = (UINT64_C(1) << 25),

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 do not see obvious usage of this event yet. It is possibly that we will need it later, but since we are not sure of whether we need it, let us not add it for now.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done — removed the event bit and the addEvent() call. It can come back together with
the consumer that actually needs it.

@arturcastiel

Copy link
Copy Markdown
Member Author

jenkins build this please

@arturcastiel

Copy link
Copy Markdown
Member Author

@GitPaean a few responses, I will continue the rest Monday.

@arturcastiel

Copy link
Copy Markdown
Member Author

jenkins build this please

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

Here are some more comments for you to address.

// numComponents is the RUNSPEC COMPS count, not part of the GPTABLE input. The
// parse sites (ScheduleStatic SOLUTION seeding and the SCHEDULE handler) reject
// a COMPS-absent deck before constructing a table, so a compositional run is a
// precondition here rather than user-facing input.

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.

The comments reflect why the commit was made to change the previous commit. In the final code, one line comment or no comments are sufficient.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Trimmed — the constructor comment is now a single intent line; the parse-site
rationale lives at the gate.

// GPTABLE implies a compositional run. COMPS (numComponents) is a RUNSPEC
// precondition for the table constructor, so reject a COMPS-absent deck here
// with a user-facing error before constructing any table.
if (numComponents < 1) {

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.

The value of COMPS is already checked when parsing COMPS. Here from the readability aspect, we only want to make sure it is at compositional mode.

   const auto& runspec = handlerContext.static_schedule().m_runspec;

    // GPTABLE only applies to a compositional run. 
    if (! runspec.compositionalMode()) {
        throw OpmInputError {
            "GPTABLE requires a compositional run (COMPS).",
            handlerContext.keyword.location()
        };
    }

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done — replaced the numComps() >= 1 guard with
if (!runspec.compositionalMode()) throw OpmInputError{"GPTABLE requires a compositional run (COMPS).", location};
as you wrote. compositionalMode() is m_comps > 0, so it is the same precondition
expressed as intent; numComps() is now used only to size the rows.

// GPTABLE implies a compositional run. COMPS (numComponents) is a RUNSPEC
// precondition for the table constructor, so reject a COMPS-absent deck here
// with a user-facing error before constructing any table.
if (!keywords.empty() && (numComponents < 1)) {

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.

Same here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done — the same compositionalMode() gate is now at the SOLUTION parse site;
gptable_solution_section takes the Runspec and derives the component count, which
also drops the static_cast<int> at the call site.

@@ -0,0 +1,24 @@
{

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.

You introduced the keyword definition, while it never got parsed or used.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

GPTDIMS is intentionally register-but-ignore, following the VFPPDIMS/VFPIDIMS precedent:
the JSON is registered so a deck carrying it parses cleanly, but there is no consumer
because the gas-plant tables — like VFPPROD/VFPINJ — live in a dynamically sized
container, so the declared maxima impose no limit. Wiring a max-dimension consumer would
reintroduce the static-allocation pattern OPM moved away from. I added a test
(GPTABLE_GPTDIMS_is_parsed_and_ignored) documenting it: a table numbered beyond
GPTDIMS's stated maximum is still accepted. Happy to drop the JSON instead if you would
rather it stay out until a consumer needs it.

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 am not familiar with this. @bska maybe you have a comment regarding this?

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.

You can still check that the table numbers don't exceed the maximum declared value and issue a diagnostic if they do.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Picked this up following @bska's suggestion below — GPTDIMS is now read and its
MAX_GAS_PLANT_TABLES drives a warning when a table number exceeds it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good idea — GPTDIMS MAX_GAS_PLANT_TABLES is now read into Runspec, and a table number
exceeding it emits a warning at both parse sites. It stays a diagnostic only: the tables
remain in a dynamically sized container, so the declared maximum does not constrain storage.
This replaces the earlier register-but-ignore handling.

*/

#ifndef OPM_GAS_PLANT_TABLE_HPP
#define OPM_GAS_PLANT_TABLE_HPP

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.

minor nitpick, you might want to header guard matching either the class name or file name.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The header guard was already OPM_GAS_PLANT_TABLE_HPP; with the class renamed it now
matches the class name.

/// heavy-component mole fraction followed by one recovery fraction per
/// component (numComponents + 1 dimensionless values per row). Parse and
/// represent only -- no gas-plant recovery physics.
class GptableTable

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.

It is a little strange name with two tables in the class name. Maybe GasPlantTable can be fine.

And then you can keep the header guard unchanged.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Renamed GptableTable -> GasPlantTable (class only; filenames unchanged). The header
guard now matches the class name.

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.

The file name should be changed.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done — renamed the files to GasPlantTable.{hpp,cpp} and updated the includes and the
CMake listing.

// and the key-column ordering required for interpolation depend on how the table
// is used, so they are left to the first consumer of the recovery data.
for (const auto& value : data) {
if (! std::isfinite(value) || (value < 0.0)) {

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 all the values should be 0. <= value <= 1.0 . It can be 0. Please double check.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done — and double-checked as you asked. Every value in the table is a normalized
fraction, so [0, 1] is the physical domain, both ends inclusive (0 and 1 are valid):

  • column 1 is the heavy-component mole fraction (the interpolation key) — a mole fraction
    is in [0, 1] by definition;
  • the remaining columns are per-component recovery fractions (the fraction of each
    component recovered to the plant liquid) — bounded by [0, 1] by mass conservation,
    since a component cannot be recovered beyond its feed amount.

The per-entry check now rejects any value < 0 or > 1, accepting exactly 0 and 1.
New test GPTABLE_value_above_one_throws.

throw OpmInputError {
fmt::format("GPTABLE table {}: every entry must be a finite, "
"non-negative value, but got {}.", table_num, value),
location

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.

When it has more than two rows, the first column needs to be strictly increasing. I do not have enough understanding for other column yet, but since they are not documented, we can leave it as it is for now.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done — the heavy mole fraction (column 1) must now be strictly increasing. I enforce it
for any table with two or more rows rather than strictly more than two: a 2-row table with
an equal or descending key is just as ill-posed for interpolation, so the invariant is the
same. The other columns are left unconstrained as you noted. New test
GPTABLE_non_monotonic_key_column_throws.

/// column index in [0, numComponents) -- it is NOT the 1-based deck component
/// number returned by lowerComponent()/upperComponent(); a caller bracketing
/// recovery columns by those indices must subtract 1.
double recovery(std::size_t row, int comp) const

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.

The component count comes from runspec.numComps() (already std::size_t), so storing m_num_components as size_t and taking the constructor arg as std::size_t removes the casts here.

comp can also be std::size_t here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done — m_num_components, the constructor argument, and the recovery() column index are
now std::size_t, matching runspec.numComps(); the casts in numRows(),
heavyMoleFraction(), recovery(), and at the two call sites are gone. As size_t,
m_num_components + 1 can no longer underflow to 0, so the numRows() divisor guard is
now just a zero-component sanity check.

@arturcastiel

Copy link
Copy Markdown
Member Author

jenkins build this please

int upperComponent() const { return this->m_upper_component; }
std::size_t numComponents() const { return this->m_num_components; }

std::size_t numRows() const

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.

a function rowWidth() can be used to remove a few m_num_components + 1. It is also more essential when coming to this class. You can wait to address this together with other comments likely coming later.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done — added a rowWidth() helper and used it in place of the repeated
m_num_components + 1.

@GitPaean

Copy link
Copy Markdown
Member

Thanks for the update. I am mostly done from my side with two small leftover comments. I will invite @bska for a quick review for this PR now. He is more familiar with this domain.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.

Comment on lines +116 to +122
// Seed every SOLUTION-section GPTABLE occurrence (each may carry several
// records), matching how the SCHEDULE handler processes all records.
for (const auto* keyword : keywords) {
for (const auto& record : *keyword) {
tables.emplace_back(record, numComponents, keyword->location());
}
}

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.

yes, this is valid. We should not allow different table numbers specified in one keyword. I assume we are allowed to overwrite between different keywords.

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.

It is indeed a valid concern and we should protect against that situation somehow, if only to enable emitting a useful diagnostic. The way we internalise the information however, through map_member<>::update(), means that we effectively resolve duplicates in favour of "last entry wins" so we never actually run with duplicate tables.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Made consistent across both paths. The SOLUTION seeding now applies the same in-keyword
duplicate handling as the SCHEDULE handler. Per @bska's note below: because GPTABLE tables are
internalized through map_member<>::update(), a repeated table number resolves last-wins and a
run never actually contains conflicting duplicate tables -- so both sites now emit an
OpmLog::warning on an in-keyword duplicate rather than throwing, which surfaces the authoring
error without a hard reject. Later occurrences in the deck still override earlier ones, as you
noted. (The SCHEDULE handler's earlier hard throw was relaxed to the same warning for parity.)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Addressed -- as a warning rather than a hard rejection, and here is the reasoning. As @bska
pointed out, GPTABLE tables are internalized through map_member<>::update(), which resolves a
repeated table number last-wins, so a run never actually ends up with two conflicting tables of
the same number. That already secures what "we should not allow it" is protecting against --
there is never an ambiguous duplicate in effect. On top of that, an in-keyword duplicate now
emits an OpmLog::warning (in both the SOLUTION seeding and the SCHEDULE handler), so the
authoring error is surfaced rather than silently swallowed. Overwrites across separate keywords
are untouched (still last-wins), as you expected. Happy to promote this to a hard error if you
would still prefer an outright reject.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Agreed — since the tables are internalized through map_member<>::update() (last-wins), a
duplicate never reaches a run, so I changed the in-keyword duplicate from a hard error to an
OpmLog::warning in both the SCHEDULE handler and the SOLUTION seeding. The deck still
parses; the test now asserts the warning path and that the last record wins.

Comment thread opm/input/eclipse/Schedule/KeywordHandlers.cpp
using Kw = Opm::ParserKeywords::GPTABLE;

std::vector<Opm::GasPlantTable> tables;

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.

should be a early exit if the keyword is not specified.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done — the SOLUTION seeding now returns early when the section carries no GPTABLE.

@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've had a brief look at this and there's more work needed. In addition to what has already been mentioned by earlier reviewers we should prefer requires over manual runtime checks where possible. I also notice one potential pitfall that this implementation doesn't handle, either by detection and rejection or by adapting its interpretation of GPTABLE::DATA

Comment on lines +398 to +404
// GPTABLE only applies to a compositional run (COMPS).
if (! runspec.compositionalMode()) {
throw OpmInputError {
"GPTABLE requires a compositional run (COMPS).",
handlerContext.keyword.location()
};
}

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.

The RUNSPEC section never changes, so this check should be expressed as a requires clause in the GPTABLE keyword model instead. BRANPROP and NODEPROP are examples of how to write such clauses.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good call — moved the compositional-run requirement to a requires: [COMPS] clause on the
GPTABLE keyword (following BRANPROP/NODEPROP) and removed the runtime compositionalMode()
checks from both the SCHEDULE handler and the SOLUTION seeding. This supersedes the explicit
C++ gate from the previous round.

Comment thread tests/parser/ScheduleTests.cpp Outdated

BOOST_AUTO_TEST_CASE(GPTABLE_default_indices_resolve_to_ncomps) {
const auto sched = make_schedule(gptable_deck(
"GPTABLE\n 1 1* 1* 0.0 0.10 0.20 0.70 /\n/\n"));

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.

Please prefer raw string literals instead of manually embedding newline characters in regular string literals. I.e., please write

const auto sched = make_schedule(gptable_deck(R"(GPTABLE
  1 1* 1*
  0.0 0.1 0.2 0.7 /
)"));

instead

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done — converted the GPTABLE test decks to raw string literals.

Comment on lines +116 to +122
// Seed every SOLUTION-section GPTABLE occurrence (each may carry several
// records), matching how the SCHEDULE handler processes all records.
for (const auto* keyword : keywords) {
for (const auto& record : *keyword) {
tables.emplace_back(record, numComponents, keyword->location());
}
}

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.

It is indeed a valid concern and we should protect against that situation somehow, if only to enable emitting a useful diagnostic. The way we internalise the information however, through map_member<>::update(), means that we effectively resolve duplicates in favour of "last entry wins" so we never actually run with duplicate tables.

@@ -0,0 +1,24 @@
{

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.

You can still check that the table numbers don't exceed the maximum declared value and issue a diagnostic if they do.

Comment on lines +90 to +91
const auto row_width = numComponents + 1;
if (data.empty() || (data.size() % row_width != 0)) {

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.

What happens if lower_component is not the same as upper_component, i.e., if upper_component is strictly greater than lower_component?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good catch — I now reject an inverted bracket at parse. lower_component and
upper_component define a range of heavy components, so I require lower <= upper (equal is
allowed -- a single component); a table with lower > upper raises an OpmInputError. The rest
of the bracket's physical interpretation is still deferred to the first consumer of the
recovery data. Let me know if E300 actually permits an inverted bracket and I will relax it.

@arturcastiel arturcastiel force-pushed the gptable-schedule-consumer branch 4 times, most recently from bd72978 to ddfc989 Compare June 25, 2026 13:56
@arturcastiel

Copy link
Copy Markdown
Member Author

jenkins build this please

@arturcastiel arturcastiel force-pushed the gptable-schedule-consumer branch from ddfc989 to 7e9f6ea Compare June 25, 2026 14:45
Add the keyword definitions for GPTABLE (the gas-plant recovery table, valid in the
SOLUTION and SCHEDULE sections, requiring a compositional run via `requires: [COMPS]`)
and GPTDIMS (its RUNSPEC dimension keyword), and register both in the keyword list.
Introduce GasPlantTable: per table number it stores the heavy-component bracket and the
recovery data (one heavy mole fraction plus one recovery fraction per component per row).
Validate at construction: table number present and positive; component indices in
[1, numComponents] forming a non-inverted bracket (lower <= upper); every entry a finite
fraction in [0, 1]; the heavy mole-fraction key column strictly increasing. The class is
parse-and-represent only -- the physical interpretation is deferred to the first consumer.
…DIMS

Internalise GPTABLE into ScheduleState via map_member (last-wins): seed SOLUTION-section
occurrences at report step 0 and apply SCHEDULE-section occurrences as time updates. A
table number repeated within a single keyword is diagnosed with a warning, not rejected.
Read GPTDIMS MAX_GAS_PLANT_TABLES into Runspec and warn when a table number exceeds it
(diagnostic only; the container is dynamically sized).
Cover SOLUTION seeding + SCHEDULE re-spec, every validation path (table number, component
bracket incl. inverted range, value range, monotonic key column), the duplicate-number and
GPTDIMS-bound diagnostics, the COMPS requires-clause rejection, and round-trip serialization.
@arturcastiel arturcastiel force-pushed the gptable-schedule-consumer branch from 7e9f6ea to f3a42d9 Compare June 25, 2026 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

manual:new-feature This is a new feature and should be described in the manual

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants