Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Write MULTPV to INIT-file #4021

Merged
merged 6 commits into from
May 13, 2024
Merged

Write MULTPV to INIT-file #4021

merged 6 commits into from
May 13, 2024

Conversation

blattms
Copy link
Member

@blattms blattms commented Apr 18, 2024 •
edited
Loading

This output was missing from the init file previously. Now MULTPV will be output if it is specified in the GRID or EDIT section.

Support for MULTPV in the SCHEDULE section will be added in an upcoming PR. I need to sort out the parallel handling of that.

This is is #4015 again

@blattms
Copy link
Member Author

blattms commented Apr 18, 2024

jenkins build this ignore_extra please

@blattms blattms added this to the Release 2024.04 milestone Apr 18, 2024
@blattms blattms force-pushed the feature/ite-multpv-to-init-v2 branch from 8dcae7f to db24d05 Compare April 22, 2024 12:29
@blattms
Copy link
Member Author

blattms commented Apr 22, 2024

jenkins build this ignore_extra please

@blattms
Copy link
Member Author

blattms commented Apr 22, 2024

Cool, finally this works with all existing tests, see jenkins #6985.

The two difference to the previous attempt are:

  • I had to move a few methods to delay instantiation to after the functions are specialised
  • In apply_multipliers (used in the EDIT section) we first check whether PORV is already initialized. Only if that is the case then MULTPV in the EDIT section will also be applied to PORV. This mimics the currenty behaviour (including issues pointed out in Inconsistent handling of PORV/MULTPV in EDIT section #4024)

@lisajulia
Copy link
Contributor

I would like to include this into the release candidate 2, so I'll wait until this is merged to master until I create the new release candidate, ok?

@bska
Copy link
Member

bska commented Apr 29, 2024

jenkins build this ignore_extra please

bska
bska reviewed Apr 29, 2024
View reviewed changes
Copy link
Member

@bska bska left a comment

Choose a reason for hiding this comment

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

Looks good for the most part. We have to be a little bit careful about what we do to the reference solutions in the time leading up to the release, but if the release manager is convinced that this should also go into the release then we're okay to update the reference solutions too.

opm/input/eclipse/EclipseState/Grid/FieldProps.cpp Outdated
@@ -814,6 +777,52 @@ bool FieldProps::has<int>(const std::string& keyword) const {
}


void FieldProps::apply_multipliers()
{
bool hasPorvBefore = (this->double_data.find(ParserKeywords::PORV::keywordName) == this->double_data.end());
Copy link
Member

Choose a reason for hiding this comment

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

Would you mind adding a comment here for why hasPorvBefore is the same as PORV not being in double_data, please? Also, is there a reason for not marking hasPorvBefore as const?

opm/input/eclipse/EclipseState/Grid/FieldProps.cpp Outdated
Comment on lines 798 to 801
using Scalar = typename std::remove_cv_t<std::remove_reference_t<decltype(iter->second.data[0])>>;
std::transform(iter->second.data.begin(), iter->second.data.end(),
mult_iter->second.data.begin(), iter->second.data.begin(),
std::multiplies<Scalar>());
Copy link
Member

Choose a reason for hiding this comment

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

I'll mention once more that this code would be simpler if you did not explicitly specify the Scalar type and instead passed std::multiplies<>() (literally) as the final argument to std::transform(). Using the defaulted template parameter (formally void) means we get template argument deduction on the parameters passed to the function call operator of std::multiplies<> and that is usually what we want.

@blattms
Copy link
Member Author

blattms commented Apr 29, 2024

I still would like to rerun my local tests for this. Sorry that this takes so long. Somehow things keep popping up...

blattms added 4 commits April 30, 2024 12:31
@blattms
Write MULTPV to INIT-file and fix a few minor issues (v2.0)
9ba5515 
This reverts commit a0e01b4 which
reverted my initial try in OPM#4015.

The problem in said PR was that one of the tests (MULTREGT-01) broke
(probably due to applying MULTPV twice).
@blattms
[fix] Apply MULTPV to PORV when in EDIT section.
169e775 
This is needed as the simulator will use PORV and assume that the
multipliers are applied.

We needed to move the function apply_multipiers to after the
specializations of the init_get functions are done, to not specialize
after the instantiation (compiler error on g++-12)
@blattms
[fix] Only apply MULTPV to PORV if it is initialized already.
23227af 
PORV is special. If it is not already stored it will be computed
based on the other properies including MULTPV in init_get.

Hence we previously might have applied multipliers multiple times.
@blattms
[fix] Make has method has been specialized before it is instantiated.
cdf3144 
Fixes compilation issue:

```
opm/input/eclipse/EclipseState/Grid/FieldProps.cpp:813:63: error: specialization of ‘bool Opm::FieldProps::has(const std::string&) const [with T = double; std::string = std::__cxx11::basic_string<char>]’ after instantiation
  813 | bool FieldProps::has<double>(const std::string& keyword_name) const {
      |                                                               ^~~~~
```
@blattms blattms force-pushed the feature/ite-multpv-to-init-v2 branch from db24d05 to 30730f9 Compare April 30, 2024 13:00
@blattms blattms removed this from the Release 2024.04 milestone Apr 30, 2024
@blattms
Copy link
Member Author

blattms commented Apr 30, 2024

I cleared the milestone. This is just too delayed.

@blattms
Copy link
Member Author

blattms commented Apr 30, 2024

jenkins build this ignore_extra please

@blattms blattms force-pushed the feature/ite-multpv-to-init-v2 branch from 30730f9 to c579404 Compare April 30, 2024 13:33
@blattms
Copy link
Member Author

blattms commented Apr 30, 2024

I checked my testcase now, too.
Review comments addressed, just not sure whether I got what the requested comment should be about.

@blattms
Copy link
Member Author

blattms commented Apr 30, 2024

jenkins build this ignore_extra please

bska
bska approved these changes May 10, 2024
View reviewed changes
Copy link
Member

@bska bska left a comment

Choose a reason for hiding this comment

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

Thank you for the updates. This looks good to me now. Unless you have other changes incoming here I suggest we update the reference solutions and merge this work into the master branch.

@blattms
Copy link
Member Author

blattms commented May 13, 2024 •
edited
Loading

Yep, let's get this in.

@blattms
Copy link
Member Author

blattms commented May 13, 2024

jenkins build this with update_data please

@jenkins4opm jenkins4opm mentioned this pull request May 13, 2024
Merged
jenkins4opm pushed a commit to jenkins4opm/opm-tests that referenced this pull request May 13, 2024
Automatic Reference Data Update for PR OPM/opm-common#4021
63bf3a1 
Reason: PR OPM/opm-common#4021

opm-common     = 465a8eba0b601149d237bcb96a17d96610899668
opm-grid       = 321ec3bc932e5f7dc000c64b75129f771122dcdf
opm-models     = d9bf4ad4d3e39829513b8b24c66a2774b47f17a1
opm-simulators = 465a8eba0b601149d237bcb96a17d96610899668

### Changed Tests ###

  * gas_precsalt
  * fpr_nonhc
  * 01_multregt
@bska
Copy link
Member

bska commented May 13, 2024

jenkins build this with opm-tests=1161 please

bska added a commit to OPM/opm-tests that referenced this pull request May 13, 2024
@bska
Merge pull request #1161 from jenkins4opm/update_opm-common_4021
f6e0b4c 
Automatic Reference Data Update for PR OPM/opm-common#4021
@bska
Copy link
Member

bska commented May 13, 2024

The new reference solutions have been installed on the CI system. Merging this now to activate the INIT file support.

@bska bska merged commit 8900b5d into OPM:master May 13, 2024
1 check passed
@blattms blattms deleted the feature/ite-multpv-to-init-v2 branch May 14, 2024 06:55
@blattms blattms mentioned this pull request May 24, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bska bska bska approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@blattms @lisajulia @bska