Stop Processing if COPYing From Undefined Source Array #4109

bska · 2024-06-17T13:33:01Z

This PR adds defined value checking to the source array from which we're copying in a COPY operation–either with the COPY or with the COPYREG keywords. We count the number of source array values which do not have the status of deck_value and if this count is positive, then we throw an OpmInputError exception which halts further input processing.

As an example of this behaviour, the (deliberately faulty) setup

EQUALS
-- SGL not defined prior to this point
  'SGL' 0.0 2* 2* 1 5 /
/

COPY
  SGL SWL 2* 2* 4 7 /
/

will generate an "Error" of the form

Error: Problem with keyword COPY
In USER_ERR_COPY_BOX_PARTIALLY_UNDEF.DATA line 195
Copying from source array SGL in BOX (1-13, 1-22, 4-7)
would generate an undefined result in 572 blocks of target array SWL.

and stop input processing.

Similarly, For operations other than assignment (EQUALS keyword), this PR ensures that the array being mutated already exists in the input. In particular, this means that a request of the form

ADD
  SGU 0.123 /
/

will halt input processing and generate an "Error" of the form

Error: Problem with keyword ADD
In USER_ERR_ADD.DATA line 202
Target array SGU must already exist when operated upon in ADD.

unless the SGU array has already been assigned earlier.

To this end, repurpose the second parameter to member function

FieldProps::try_get<>()

and make this a bitmask of flags instead of a single bool. The currently supported flags are AllowUnsupported and MustExist
with the former representing the original 'bool' parameter and the second requesting that the property array must already exist for the request to succeed.

bska · 2024-06-17T13:33:53Z

Note: I am creating this PR in draft mode because it depends on, and contains, the earlier PR #4103. I will keep the PR in a draft state until it is ready for review and merging.

bska · 2024-06-18T18:15:17Z

jenkins build this please

bska · 2024-08-19T10:37:40Z

jenkins build this please

bska · 2024-08-19T14:31:11Z

I have corrected the unexpected PORV related problem that presented the last time I updated this PR. I now consider this to be functionally complete and I'm marking it as "ready for review".

bska · 2024-08-20T08:38:12Z

jenkins build this please

akva2

Two small suggestions that you may or may not take.

akva2 · 2024-08-20T08:29:49Z

opm/input/eclipse/EclipseState/Grid/FieldProps.cpp

+ const T scalar_value,
+ const std::vector<Box::cell_index>& index_list)
+{
+ if (op == Fieldprops::ScalarOperation::EQUAL) {


akva2 · 2024-08-20T08:38:16Z

opm/input/eclipse/EclipseState/Grid/FieldProps.hpp

 const Fieldprops::FieldData<T>& field_data() const
 {
 this->verify_status();
 return *this->data_ptr;
 }

+ /// Property validity predicate.
+ ///
+ /// Returns true if propery exists and has well defined data


suggestion: s/well/properly or the likes. the term 'well' is a bit reserved in the context. also 'property'

akva2 · 2024-08-20T10:20:52Z

opm/input/eclipse/EclipseState/Grid/FieldProps.hpp

@@ -492,7 +492,7 @@ class FieldProps {

 /// Property validity predicate.
 ///
- /// Returns true if propery exists and has well defined data
+ /// Returns true if propery exists and has fully defined data


bska · 2024-08-20T11:21:30Z

Two small suggestions that you may or may not take.

Thanks a lot for looking at this. I've pushed an update to address your comments. I'm always a little on the fence when it comes to switching on enums because while the set of valid enumerators is fixed, and typically small, the possible enumeration values are of course all of the values allowed by the underlying arithmetic type. That said, I replaced the if/else chain with a switch and a throw for unsupported enumeration values.

bska · 2024-08-20T11:21:37Z

jenkins build this please

bska · 2024-08-20T13:49:27Z

jenkins build this please

akva2 · 2024-08-20T16:19:21Z

jenkins build this please

This overload includes more context and enables generating more descriptive diagnostic messages.

This commit makes the backend implementations of ADD, MULTIPLY, &c capable of halting processing if we encounter any undefined source elements. We nevertheless maintain the definition of "defined" as value::has_value() (from the value_status.hpp header), at least for the time being, as changing the definition requires making more parts of the array processing aware of the currently active section. The new helper function reject_undefined_operation() will throw an OpmInputError exception with (hopefully) enough context that fixing the input will be easier. While here, make the apply() function private to the FieldProps.cpp implementation file. It was already marked 'static' and does not need access to any of the FieldProps data members.

This commit adds defined value checking to the source array from which we're copying in a COPY operation--either with the COPY or with the COPYREG keywords. We count the number of source array values which do not have the status of 'deck_value' and if this count is positive, then we throw an OpmInputError exception which halts further input processing. As an example of this behaviour, the (deliberately faulty) setup EQUALS -- SGL not defined prior to this point 'SGL' 0.0 2* 2* 1 5 / / COPY SGL SWL 2* 2* 4 7 / / will generate an "Error" of the form Error: Problem with keyword COPY In USER_ERR_COPY_BOX_PARTIALLY_UNDEF.DATA line 195 Copying from source array SGL in BOX (1-13, 1-22, 4-7) would generate an undefined result in 572 blocks of target array SWL. and stop input processing.

For operations other than assignment (EQUALS keyword), this commit ensures that the array being mutated already exists in the input. In particular, this means that a request of the form ADD SGU 0.123 / / will halt input processing and generate an "Error" of the form Error: Problem with keyword ADD In USER_ERR_ADD.DATA line 202 Target array SGU must already exist when operated upon in ADD. unless the 'SGU' array has already been assigned earlier. To this end, repurpose the second parameter to member function FieldProps::try_get<>() and make this a bitmask of flags instead of a single 'bool'. The currently supported flags are 'AllowUnsupported' and 'MustExist' with the former representing the original 'bool' parameter and the second requesting that the property array must already exist for the request to succeed.

In particular, 1. Replace if/else chain by switch 2. Replace 'well formed' with 'fully defined' in documentation

bska · 2024-08-20T16:54:43Z

jenkins build this please

bska · 2024-08-20T18:05:42Z

PR approved and build check is green. I'll merge into master.

The existence checks introduced in commit 90be0956d0 (PR OPM#4109) turned out to be a little too restrictive. Recent experience suggests that we need to permit array operations like ADD or MULTIPLY on the MULT* arrays (MULT[XYZ] and MULT[XYZ]-) even if these arrays have not yet been explicitly defined in the input deck. This commit re-enables those array operations for property arrays whose 'multiplier' flag is 'true'.

The existence checks introduced in commit 90be095 (PR OPM#4109) turned out to be a little too restrictive. Recent experience suggests that we need to permit array operations like ADD or MULTIPLY on the MULT* arrays (MULT[XYZ] and MULT[XYZ]-) even if these arrays have not yet been explicitly defined in the input deck. This commit re-enables those array operations for property arrays whose 'multiplier' flag is 'true'.

bska force-pushed the copy-undef-src-array branch 5 times, most recently from ce5ee02 to 5d323c5 Compare June 18, 2024 18:14

bska force-pushed the copy-undef-src-array branch 8 times, most recently from 3553c46 to aebab88 Compare June 26, 2024 07:37

bska force-pushed the copy-undef-src-array branch 10 times, most recently from 4d7b24a to fced603 Compare July 2, 2024 16:05

bska force-pushed the copy-undef-src-array branch 5 times, most recently from 1699f86 to 67b9603 Compare July 9, 2024 10:58

bska force-pushed the copy-undef-src-array branch 4 times, most recently from 66ff822 to bcf1e6a Compare August 19, 2024 10:27

bska marked this pull request as ready for review August 19, 2024 14:31

bska force-pushed the copy-undef-src-array branch from bcf1e6a to b18b142 Compare August 20, 2024 08:27

akva2 approved these changes Aug 20, 2024

View reviewed changes

akva2 reviewed Aug 20, 2024

View reviewed changes

bska force-pushed the copy-undef-src-array branch from f6398f0 to b253f3f Compare August 20, 2024 10:24

bska force-pushed the copy-undef-src-array branch from b253f3f to d75d747 Compare August 20, 2024 13:48

bska added 5 commits August 20, 2024 18:53

Add New Status Verification Overload to FieldDataManager

3637cf7

This overload includes more context and enables generating more descriptive diagnostic messages.

Address PR Review Comments

eb23b95

In particular, 1. Replace if/else chain by switch 2. Replace 'well formed' with 'fully defined' in documentation

bska force-pushed the copy-undef-src-array branch from d75d747 to eb23b95 Compare August 20, 2024 16:54

bska merged commit db97e3f into OPM:master Aug 20, 2024
1 check passed

bska deleted the copy-undef-src-array branch August 20, 2024 18:05

bska mentioned this pull request Sep 26, 2024

Permit Operating on Defaulted MULT* Arrays #4235

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stop Processing if COPYing From Undefined Source Array #4109

Stop Processing if COPYing From Undefined Source Array #4109

bska commented Jun 17, 2024

bska commented Jun 17, 2024

bska commented Jun 18, 2024

bska commented Aug 19, 2024

bska commented Aug 19, 2024

bska commented Aug 20, 2024

akva2 left a comment

akva2 Aug 20, 2024

akva2 Aug 20, 2024 •

edited

Loading

akva2 Aug 20, 2024

bska commented Aug 20, 2024

bska commented Aug 20, 2024

bska commented Aug 20, 2024

akva2 commented Aug 20, 2024

bska commented Aug 20, 2024

bska commented Aug 20, 2024

Stop Processing if COPYing From Undefined Source Array #4109

Stop Processing if COPYing From Undefined Source Array #4109

Conversation

bska commented Jun 17, 2024

bska commented Jun 17, 2024

bska commented Jun 18, 2024

bska commented Aug 19, 2024

bska commented Aug 19, 2024

bska commented Aug 20, 2024

akva2 left a comment

Choose a reason for hiding this comment

akva2 Aug 20, 2024

Choose a reason for hiding this comment

akva2 Aug 20, 2024 • edited Loading

Choose a reason for hiding this comment

akva2 Aug 20, 2024

Choose a reason for hiding this comment

bska commented Aug 20, 2024

bska commented Aug 20, 2024

bska commented Aug 20, 2024

akva2 commented Aug 20, 2024

bska commented Aug 20, 2024

bska commented Aug 20, 2024

akva2 Aug 20, 2024 •

edited

Loading