Separate out geomechanical items from BCPROP to new keyword BCMECH - #5295
Conversation
svenn-t
added
the
manual:new-feature
|
jenkins build this opm-simulators=7326 please |
|
I think it is fine, it will give minimal changes in decks. |
There was a problem hiding this comment.
🟡 Changes recommended
The current BCState merge/update logic makes BCMECH + BCPROP interaction order-dependent and can leave mechanical state unapplied when multiple BCPROP components share the same INDEX.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR introduces a new BCMECH SCHEDULE keyword to carry geomechanical boundary-condition items previously embedded in BCPROP, and renames the parsing/state container from BCProp to BCState to reflect that it now aggregates state from multiple keywords.
Changes:
- Added
BCMECHkeyword definition and registered a new schedule handler to parse it intoScheduleState::bcstate. - Refactored
BCPropintoBCStateand updated schedule state storage/serialization and tests accordingly. - Removed geomechanics-related items from the
BCPROPkeyword definition and updated error messaging/documentation text.
File summaries
| File | Description |
|---|---|
| tests/parser/ScheduleTests.cpp | Updated schedule test access from bcprop to bcstate. |
| tests/parser/BCConfigTests.cpp | Updated BC parsing tests to use BCState and added a BCMECH parsing case. |
| opm/input/eclipse/share/keywords/keyword_list.cmake | Registered new BCMECH keyword file for generation/build. |
| opm/input/eclipse/share/keywords/900_OPM/B/BCPROP | Removed geomechanics items from BCPROP definition. |
| opm/input/eclipse/share/keywords/900_OPM/B/BCMECH | Added new BCMECH keyword definition. |
| opm/input/eclipse/Schedule/ScheduleState.hpp | Renamed stored boundary-condition state from bcprop to bcstate and updated serialization. |
| opm/input/eclipse/Schedule/KeywordHandlers.cpp | Added BCMECH handler and updated BCPROP handler to target bcstate. |
| opm/input/eclipse/Schedule/BCState.hpp | Introduced BCState API and new updateBCMech entry point. |
| opm/input/eclipse/Schedule/BCState.cpp | Implemented split parsing (fromBCProp / fromBCMech) and update logic. |
| opm/input/eclipse/EclipseState/SimulationConfig/BCConfig.cpp | Updated obsolete-keyword error message to reference BCPROP/BCMECH. |
| CMakeLists_files.cmake | Switched build inputs from BCProp.* to BCState.*. |
Review details
Suppressed comments (2)
opm/input/eclipse/Schedule/BCState.cpp:212
- BCMECH updates are only applied to the first existing BCFace with the matching index. If multiple BCPROP records exist for the same INDEX (different COMPONENT), this leaves the remaining faces with stale/default mechanical state, and it also makes BCMECH+BCPROP order-dependent.
opm/input/eclipse/Schedule/BCState.cpp:202 - When a new BCFace is created from BCPROP, it doesn't inherit any existing BCMECH state for the same INDEX. This breaks the intended behavior of allowing BCMECH and BCPROP to be used together (and makes parsing order-dependent when BCMECH appears before BCPROP).
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| BCMECH | ||
| 1 FIXED 1 0 0 1.0 * * 2.0 * * / | ||
| 2 FIXED 0 1 0 * 3.0 * / | ||
| 3 FREE 0 0 1 * * 4.0 * * 5.0 / | ||
| / |
There was a problem hiding this comment.
Added new test with BCPROP and BCMECH in the same deck. This revealed a small bug with parsing: If BCMECH was before BCPROP, both defining on same index, BCPROP would duplicate entry, and possibly that info would have been lost when queried later. Now it adds the BCPROP info to the index instead.
|
I also suggest adding a BCMECH regression test. Or add BCMECH to one of the TPSA tests. I assume it doesn't have it since Jenkins didn't complain about the keyword change. |
Added a new TPSA case in opm-tests with BCMECH (OPM/opm-tests#1583), as well as including it in the regression tests in the opm-simulators companion PR. |
|
jenkins build this opm-simulators=7326 please |
- Add separate handling of BCPROP and BCMECH to allow for one without the other - Renamed BCProp class to BCState since it still contain BC info on both
|
jenkins build this opm-simulators=7326 please |
|
The test case fails as expected. I will create the ref solution. |
|
jenkins build this opm-simulators=7326 update_data please |
|
jenkins build this opm-simulators=7326 opm-tests=1591 please |
|
Thanks. With the regression ref. in place, I will start the merging process. |
PR OPM/opm-simulators#7326 Reason: PR OPM/opm-common#5295 PR OPM/opm-simulators#7326 opm-common = 9ba0983324e69a3af878d0874840202d268bc0f4 opm-grid = 798166c5de06f16cc4d9d0acd19d15cc681f6ffd opm-simulators = 016e665c5afd36e61767223fc0fb702f5f9f0884 ### Changed Tests ### * tpsa_bc
…simulators_7326 Automatic Reference Data Update for PR OPM/opm-common#5295
This PR separates out the items for defining geomechanics boundary condition from BCPROP to a new keyword BCMECH. This allows for both keywords to be used together in a deck, or defining one without the other.
Note that the class where both keywords are parsed is renamed from BCProp to BCState to avoid confusion, since it now parses two keywords, BCPROP and BCMECH.