Deprecate Action RST Dimension Function#5238
Open
bska wants to merge 1 commit into
Open
Conversation
bska
added
the
manual:irrelevant
Member
Author
|
jenkins build this please |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR deprecates the legacy “linear array of unnamed indices” representation for ACTION restart dimensions and shifts allocation/stride computation to named entriesPer*() helper functions, while also hardening Actdims with clearer semantics, documentation, and safer arithmetic.
Changes:
- Expose and use named
entriesPer*()helpers for ACTIONX restart array sizing/strides; deprecatecreateActionRSTDims(). - Refactor
AggregateActionxDataallocation to avoid passing around the legacy dims vector. - Improve
Actdimswith range checking, Doxygen docs, and a saferline_size()implementation.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_AggregateActionxData.cpp | Updates tests to use entriesPer*() stride helpers instead of indexing into a dims vector. |
| opm/output/eclipse/WriteRestartHelpers.hpp | Publishes entriesPerLine()/entriesPerZLACT() and adds Doxygen + deprecation annotation for legacy dims API. |
| opm/output/eclipse/CreateActionRSTDims.cpp | Implements the public entriesPer*() helpers and keeps (now-deprecated) createActionRSTDims() as a wrapper. |
| opm/output/eclipse/AggregateActionxData.hpp | Updates internal constructor signature to remove dependency on legacy dims vector. |
| opm/output/eclipse/AggregateActionxData.cpp | Allocates ACTIONX arrays using entriesPer*() and num_actions rather than the legacy dims vector. |
| opm/input/eclipse/Schedule/Action/Actdims.hpp | Adds Doxygen docs and inlines simple accessors for ACTDIMS fields. |
| opm/input/eclipse/Schedule/Action/Actdims.cpp | Adds ACTDIMS range validation and fixes line_size() to avoid mixed-type std::div() usage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bska
force-pushed
the
deprecate-action-rst-dimfunc
branch
from
8e0cb9d to
d653608
Compare
bska
force-pushed
the
deprecate-action-rst-dimfunc
branch
from
bbd7081 to
1b5d0b6
Compare
akva2
approved these changes
Jul 2, 2026
akva2
left a comment
akva2
left a comment
Member
There was a problem hiding this comment.
LGTM. A couple of very petty observations.
bska
force-pushed
the
deprecate-action-rst-dimfunc
branch
from
1b5d0b6 to
8924cd7
Compare
Member
Author
Not petty at all–clear improvements of the patch. |
bska
force-pushed
the
deprecate-action-rst-dimfunc
branch
from
8665f35 to
586e007
Compare
bska
force-pushed
the
deprecate-action-rst-dimfunc
branch
from
586e007 to
2dc4f4c
Compare
bska
force-pushed
the
deprecate-action-rst-dimfunc
branch
from
2dc4f4c to
f1b0782
Compare
bska
force-pushed
the
deprecate-action-rst-dimfunc
branch
from
46c3037 to
8a3cdb6
Compare
This commit moves away from representing restart file action dimensions as a linear array with unnamed indices and instead switches to using the named helper functions entriesPer*() as a means of allocating the individual ACTIONX restart file arrays. This, in turn, requires making the entriesPerLine() and entriesPerZLACT() helper functions public instead of private in CreateActionRSTDims.cpp. While here, simplify the implementations of these functions and fix a latent bug in Actdims::line_size() which used std::div() with mixed mode arithmetic (std::size_t and int) for which there is no direct overload in the standard library. We also introduce range checking for the various ACTDIMS items and add Doxygen style documentation to class Actdims and the entriesPer*() functions. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
bska
force-pushed
the
deprecate-action-rst-dimfunc
branch
from
8a3cdb6 to
46deda8
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR moves away from representing restart file action dimensions as a linear array with unnamed indices and instead switches to using the named helper functions
entriesPer*()as a means of allocating the individualACTIONXrestart file arrays. This, in turn, requires making theentriesPerLine()andentriesPerZLACT()helper functions public instead of private inCreateActionRSTDims.cpp.While here, simplify the implementations of these functions and fix a latent bug in
Actdims::line_size()which usedstd::div()with mixed mode arithmetic (std::size_tandint) for which there is no direct overload in the standard library. We also introduce range checking for the variousACTDIMSitems and add Doxygen style documentation to classActdimsand theentriesPer*()functions.