New SummaryNode implementation by wito · Pull Request #1609 · OPM/opm-common

wito · 2020-03-19T14:36:56Z

This feature implements

Opm::EclIO::SummaryNode
Related changes to Opm::out::Summary

Opm::EclIO::SummaryNode is a lighter and more focused value object style struct than Opm::SummaryConfigNode, suitable for use in the Summary reader and writer arenas.

wito · 2020-03-19T14:37:29Z

jenkins build this please

joakim-hove · 2020-03-19T21:19:58Z

jenkins build this please

bska

I can't approve this. The changes to Summary.cpp don't serve a purpose as far as I can tell. Do just the minimum needed to make it compile and leave the rest alone.

src/opm/output/eclipse/Summary.cpp

joakim-hove · 2020-03-20T08:41:50Z

jenkins build this please

wito · 2020-03-20T08:43:01Z

@joakim-hove That's not really any point right now; there's a test error that I'm in the process of fixing.

wito · 2020-03-20T08:59:53Z

jenkins build this please

wito · 2020-03-20T10:58:32Z

jenkins build this please

wito · 2020-03-20T11:07:26Z

@joakim-hove I'm having an issue getting this to build in Jenkins; something about the build system, I think, but I'm not entirely sure what I could have done to upset it. 🙁

wito · 2020-03-20T11:11:05Z

jenkins build this please

wito · 2020-03-20T11:30:49Z

@joakim-hove Looking into it, it seems as this will resolve itself once #1610 is resolved and I can rebase to the new head.

bska · 2020-03-20T11:34:26Z

it seems as this will resolve itself once #1610 is resolved and I can rebase to the new head.

You need to rebase in any case. PR #1592 introduced a header and other changes in Summary.cpp.

wito · 2020-03-20T11:35:17Z

it seems as this will resolve itself once #1610 is resolved and I can rebase to the new head.

You need to rebase in any case. PR #1592 introduced a header and other changes in Summary.cpp.

Indeed I do, but as I'm working on macOS I can't really do so until #1610 is resolved.

akva2 · 2020-03-20T11:37:01Z

please confirm #1616 takes care of it

wito · 2020-03-20T13:28:22Z

jenkins build this please

wito · 2020-03-20T14:39:32Z

jenkins build this please

joakim-hove · 2020-03-23T16:17:32Z

@bska: You had some - reasonable - comments to this PR; could you take a new look and see if it can be merged now?

bska

This is much better. I just have a couple of more comments and then I think this will be ready for merging.

opm/io/eclipse/SummaryNode.hpp

bska · 2020-03-24T10:25:47Z

src/opm/io/eclipse/SummaryNode.cpp

+    const bool matched     { std::regex_match(keyword, user_defined_regex) } ;
+    const bool blacklisted { udq_blacklist.find(keyword) != udq_blacklist.end() } ;
+
+    return matched && !blacklisted;


Did you consider writing this as

return !blacklisted && std::regex_match(keyword, user_defined_regex);

? That way, you don't incur the cost of regular expression matching for blacklisted keywords.

I'm not sure, in this case, if that's worth it, given the domain of the function and the relative simplicity of the regular expression.

src/opm/output/eclipse/Summary.cpp

bska · 2020-03-24T10:48:19Z

One more thing. I didn't expect to see SummaryNode.[hc]pp being added to PUBLIC_*_FILES twice. I guess it covers the case

ENABLE_ECL_INPUT AND (NOT ENABLE_ECL_OUTPUT)

but it still was surprising to me. Any suggestion on that aspect, @akva2?

joakim-hove · 2020-03-24T11:01:51Z

This is much better. I just have a couple of more comments and then I think this will be ready for merging.

Thank you for getting back to this - we will fix the remaining points you raise.

joakim-hove · 2020-03-24T11:05:56Z

I didn't expect to see SummaryNode.[hc]pp being added to PUBLIC_*_FILES twice.

I was also surprised by that; I concluded it was correct but would be happy to be proven wrong. Time to revisit: #1129?

…a vectors.

bska · 2020-03-26T08:38:17Z

jenkins build this please

bska

Thanks for the updates. This looks good to me now. I'll merge into master.

bska requested changes Mar 19, 2020

View reviewed changes

src/opm/output/eclipse/Summary.cpp Outdated Show resolved Hide resolved

src/opm/output/eclipse/Summary.cpp Outdated Show resolved Hide resolved

src/opm/output/eclipse/Summary.cpp Outdated Show resolved Hide resolved

src/opm/output/eclipse/Summary.cpp Outdated Show resolved Hide resolved

wito force-pushed the new-summarynode branch from 84fcbab to da79944 Compare March 20, 2020 08:59

wito added 16 commits March 20, 2020 14:08

Adds Opm::EclIO::SummaryNode::Type::Mode.

402f28f

Removes extraneous names.

3af0bbf

Adds conversion operator SummaryConfigNode => SummaryNode.

e5e0a49

Removes extraneous type.

e317b96

Reworks to unified initialisation.

90a2eeb

Renames variables for clarity.

0265b7f

Implements Opm::EclIO::SummaryNode::unique_key().

861c098

Minor formatting tweaks.

dbf17af

Makes fallthrough explicit.

c3de96b

Fixes order of accumulation for key parts.

d9e1d39

Adds a test for Opm::EclIO::SummaryNode::unique_key()

4bb453d

Review Action: Renames Opm::EclIO::SummaryNode::name => wgname

84c57ff

Simplifies namespace declaration.

001877d

Rewrites ::need_wells() for Opm::EclIO::SummaryNode.

b940d81

Reworks ::updateValue for Opm::EclIO::SummaryNode.

cb0f27c

Minor formatting tweak.

b12397c

wito added 8 commits March 20, 2020 14:08

Reworks ::requiredSegmentVectors() for Opm::EclIO::SummaryNode.

35afdd5

Reworks requiredRestartVectors for Opm::EclIO::SummaryNode.

99375eb

Removes use of default:

62983db

Adds Opm::EclIO::SummaryNode::is_user_defined().

327fe79

Reworks Evaluator::Factory for Opm::EclIO::SummaryNode.

89fe3c3

Relocates new test in CMakeLists_files.cmake.

eba0f3d

Reworks mode vectors for Opm::EclIO::SummaryNode.

06eeae8

Refactors out duplicated code.

f884c90

wito force-pushed the new-summarynode branch from 64f0621 to f884c90 Compare March 20, 2020 13:25

Uses correct name in reference to enum value.

fb4f026

wito marked this pull request as ready for review March 20, 2020 13:27

Fixes minor formatting glitch in the SummaryNode copyright statement.

ab71b43

wito force-pushed the new-summarynode branch 3 times, most recently from a36e5ce to 3bd4769 Compare March 20, 2020 14:31

Removes various minor edits.

2c74b09

wito force-pushed the new-summarynode branch from 3bd4769 to 2c74b09 Compare March 20, 2020 14:34

bska reviewed Mar 24, 2020

View reviewed changes

Review Actions: Un-consts SummaryNode members and un-statics some dat…

06d317d

…a vectors.

wito mentioned this pull request Mar 26, 2020

Add Aquifer Category #1639

Merged

bska approved these changes Mar 26, 2020

View reviewed changes

bska merged commit 5a701a2 into OPM:master Mar 26, 2020

wito deleted the new-summarynode branch March 26, 2020 11:47

Conversation

wito commented Mar 19, 2020

Uh oh!

wito commented Mar 19, 2020

Uh oh!

joakim-hove commented Mar 19, 2020

Uh oh!

bska left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joakim-hove commented Mar 20, 2020

Uh oh!

wito commented Mar 20, 2020

Uh oh!

wito commented Mar 20, 2020

Uh oh!

wito commented Mar 20, 2020

Uh oh!

wito commented Mar 20, 2020

Uh oh!

wito commented Mar 20, 2020

Uh oh!

wito commented Mar 20, 2020

Uh oh!

bska commented Mar 20, 2020

Uh oh!

wito commented Mar 20, 2020

Uh oh!

akva2 commented Mar 20, 2020

Uh oh!

wito commented Mar 20, 2020

Uh oh!

wito commented Mar 20, 2020

Uh oh!

joakim-hove commented Mar 23, 2020

Uh oh!

bska left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bska Mar 24, 2020

Choose a reason for hiding this comment

Uh oh!

wito Mar 26, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bska commented Mar 24, 2020

Uh oh!

joakim-hove commented Mar 24, 2020

Uh oh!

joakim-hove commented Mar 24, 2020

Uh oh!

bska commented Mar 26, 2020

Uh oh!

bska left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants