salt solubility table allowing specification of solubility per region #2593

plgbrts · 2021-08-04T11:56:23Z

No description provided.

akva2 · 2021-08-04T11:58:45Z

opm/parser/eclipse/EclipseState/Tables/SaltSolubilityTable.hpp

+
+namespace Opm {
+
+    class DeckItem;


did you mean to forward DeckRecord here?

Also in general we do not indent for namespace. Maybe whatever you used as blueprint did but please do not replicate the mistake.

opm/parser/eclipse/EclipseState/Tables/SaltSolubilityTable.hpp

akva2 · 2021-08-04T12:43:10Z

jenkins build this please

src/opm/parser/eclipse/EclipseState/Tables/TableManager.cpp

joakim-hove · 2021-08-05T06:51:18Z

Many other tables are managed by the SimpleTableContainer which handles the region based lookup; I think you should look into whether that machinery can be used also in this situation.

If you decide it is best to stick with the current implementation - that is OK to, but maybe comment on it.

plgbrts · 2021-11-29T14:39:05Z

I followed for this table the SDENSITY table implementation which has the same structure. This PR is needed for coming PRs of @goncalvesmachadoc and myself. Would it be OK to merge as it is so we can go ahead?

joakim-hove · 2021-11-29T15:58:31Z

src/opm/parser/eclipse/EclipseState/Tables/SaltSolubilityTable.cpp

+
+#include <vector>
+#include <opm/parser/eclipse/Deck/DeckItem.hpp>
+#include <opm/parser/eclipse/Deck/DeckKeyword.hpp>


Don't need the DeckKeyword include

joakim-hove · 2021-11-29T16:10:44Z

src/opm/parser/eclipse/EclipseState/Tables/TableManager.cpp

@@ -897,6 +902,10 @@ std::optional<JFunc> make_jfunc(const Deck& deck) {
        return getTables("SALTPVD");
    }

+   const TableContainer& TableManager::getSaltsolTables() const {


Will this work? Does not look like this tablecontainer is initialized at all.

joakim-hove · 2021-11-29T16:11:58Z

src/opm/parser/eclipse/EclipseState/Tables/TableManager.cpp

+        size_t numTables = m_tabdims.getNumPVTTables();
+        saltsoltables.resize(numTables);
+
+        const auto& keyword = deck.getKeyword("SALTSOL");


This will not compile any longer:

#include <opm/parser/eclipse/Parser/ParserKeywords/S.hpp> ... const auto& keyword = deck.get<ParserKeywords::S::SALTSOL>().back(); ...

joakim-hove · 2021-11-29T16:27:55Z

This PR is needed for coming PRs of @goncalvesmachadoc and myself. Would it be OK to merge as it is so we can go ahead?

I must say I find this a bit amusing; I suggest a change which I think is an improvement and in particular aligning with how most of the other tables are implemented[1]. Then after nearly four months of silence you come back and ask if it can be merged unmodified. Have you considered my suggestion? Maybe it was a poor suggestion after all.

If you still prefer the current implementation you should at least add some tests.

[1]: The TableContainer class implements the region based lookup, where you will automatically go the proceeding region number if the region you query for is not defined - i.e. this would be valid in Eclipse:

TABDIMS
   NTPVT=3 /
...
...
SALTSOL
  1 2 3 4 5 /
/
/

and the code should automatically work with the first table for all regions - that will not work out of the box with the current code.

joakim-hove · 2021-11-29T16:42:33Z

jenkins build this please

joakim-hove · 2021-11-30T16:59:37Z

I have been encouraged to withdraw from this PR - please disregard all my previous comments and wait for someone else to review and merge it.

Joakim

goncalvesmachadoc · 2021-11-30T17:09:06Z

We appreciate your comments explaining the benefits of changing approach. We are working on modifying the table to use the SimpleTableContainer framework.

plgbrts · 2021-12-02T15:53:58Z

The table implementation is modified following the suggestion of @joakim-hove to use the SimpleTableContainer setup

bska

I will review this more carefully later, but I noticed a couple of issues that, at least superficially, look unexpected. I'm probably missing something though, so please correct my understanding.

src/opm/parser/eclipse/EclipseState/Tables/TableManager.cpp

plgbrts · 2021-12-02T17:14:19Z

Thanks @bska for catching the accidental mistakes. I fixed them.

bska

I think this looks good in general, and I'd like to get it into the master branch. I do however have a couple of questions regarding the table schema for the multirecord version of SALTSOL. In particular, I wonder what the most general structure of the keyword will be in the case of NTPVT > 1 and how that structure is reflected in the JSON keyword representation.

bska · 2021-12-06T15:46:17Z

src/opm/parser/eclipse/share/keywords/900_OPM/S/SALTSOL

+    {
+      "name": "DATA",
+      "value_type": "DOUBLE",
+      "size_type": "ALL",


@joakim-hove will know the exact details, but I believe entering size_type: ALL here means that it is possible to parse a table with the following structure (assume NTPVT = 3)

SALTSOL 0.1 0.2 0.3 / / 0.123 /

Is that intentional? If not, what is the expected structure of a fully general SALTSOL table?

For NTPVT = 3, we would want to have:

SALTSOL 0.1 / 0.2 / 0.3 /

The table you (@bska ) mentioned is indeed not intended.
What I understood from @joakim-hove 's comment earlier that for NTPVT=3 also the table

SALTSOL 0.1 / / /

is allowed for which region 2 and 3 takes the value of region 1.
As far I can see all tables use "ALL" for size_type . Is that observation correct?

Is that observation correct?

Yes, you're right. There may be an exception somewhere that I've overlooked, but using size_type = "ALL" does appear to be the typical case.

bska · 2021-12-06T15:48:09Z

tests/parser/SaltTableTests.cpp

+        "550 0.5/\n"
+        "\n"
+        "SALTSOL\n"
+        "8.0/\n"


This test data has a single record for SALTSOL, but NTPVT = 2. Is that supposed to work? Would you mind adding an example case that uses a fully general SALTSOL table?

Here

"TABDIMS\n" "1 1/\n"

So shouldn't be NTPVT = 1 in this test case?

So shouldn't be NTPVT = 1 in this test case?

Yes, you're right. My mistake. I was confused by the layout of the RWGSALT table data.

bska

Thank you for adding a test case with NTPVT > 1. This looks good to me now. I'll run a final build check and then merge once it comes back clear.

bska · 2021-12-06T20:42:13Z

tests/parser/SaltTableTests.cpp

+        "550 0.5/\n"
+        "\n"
+        "SALTSOL\n"
+        "8.0/\n"


So shouldn't be NTPVT = 1 in this test case?

Yes, you're right. My mistake. I was confused by the layout of the RWGSALT table data.

bska · 2021-12-06T20:50:17Z

src/opm/parser/eclipse/share/keywords/900_OPM/S/SALTSOL

+    {
+      "name": "DATA",
+      "value_type": "DOUBLE",
+      "size_type": "ALL",


Is that observation correct?

Yes, you're right. There may be an exception somewhere that I've overlooked, but using size_type = "ALL" does appear to be the typical case.

bska · 2021-12-06T20:52:38Z

jenkins build this please

bska · 2021-12-07T09:00:49Z

jenkins build this please

bska · 2021-12-08T14:14:32Z

jenkins build this please

bska · 2021-12-08T14:52:12Z

I'll run a final build check and then merge once it comes back clear.

Build check is clear. I'll merge into master. Thanks a lot.

akva2 reviewed Aug 4, 2021

View reviewed changes

opm/parser/eclipse/EclipseState/Tables/SaltSolubilityTable.hpp Outdated Show resolved Hide resolved

joakim-hove reviewed Aug 5, 2021

View reviewed changes

src/opm/parser/eclipse/EclipseState/Tables/TableManager.cpp Outdated Show resolved Hide resolved

joakim-hove reviewed Nov 29, 2021

View reviewed changes

plgbrts added 5 commits December 2, 2021 11:52

salt solubility table allowing specification of solubility per region

e40fdf7

some formatting

1239649

removed because changing to SimpleTableContainer implementation

4445092

salt solubility table implementation using simpleTableContainer

f053ba9

add test for salt solubility table

3ead0f4

plgbrts force-pushed the salt_prec branch from 9764d7c to 3ead0f4 Compare December 2, 2021 15:45

bska reviewed Dec 2, 2021

View reviewed changes

src/opm/parser/eclipse/EclipseState/Tables/TableManager.cpp Outdated Show resolved Hide resolved

src/opm/parser/eclipse/EclipseState/Tables/TableManager.cpp Outdated Show resolved Hide resolved

src/opm/parser/eclipse/EclipseState/Tables/TableManager.cpp Outdated Show resolved Hide resolved

fix: number of tables should be based on NTPVT

10f11f9

bska reviewed Dec 6, 2021

View reviewed changes

added test for solubility table for 2 regions

48124f1

bska approved these changes Dec 6, 2021

View reviewed changes

bska merged commit 84bf899 into OPM:master Dec 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

salt solubility table allowing specification of solubility per region #2593

salt solubility table allowing specification of solubility per region #2593

plgbrts commented Aug 4, 2021

akva2 Aug 4, 2021

akva2 commented Aug 4, 2021

joakim-hove commented Aug 5, 2021 •

edited

Loading

plgbrts commented Nov 29, 2021

joakim-hove Nov 29, 2021

joakim-hove Nov 29, 2021

joakim-hove Nov 29, 2021 •

edited

Loading

joakim-hove commented Nov 29, 2021

joakim-hove commented Nov 29, 2021

joakim-hove commented Nov 30, 2021

goncalvesmachadoc commented Nov 30, 2021

plgbrts commented Dec 2, 2021

bska left a comment

plgbrts commented Dec 2, 2021

bska left a comment

bska Dec 6, 2021

goncalvesmachadoc Dec 6, 2021 •

edited by bska

Loading

plgbrts Dec 6, 2021

bska Dec 6, 2021

bska Dec 6, 2021

goncalvesmachadoc Dec 6, 2021 •

edited by bska

Loading

bska Dec 6, 2021

bska left a comment

bska Dec 6, 2021

bska Dec 6, 2021

bska commented Dec 6, 2021

bska commented Dec 7, 2021

bska commented Dec 8, 2021

bska commented Dec 8, 2021

salt solubility table allowing specification of solubility per region #2593

salt solubility table allowing specification of solubility per region #2593

Conversation

plgbrts commented Aug 4, 2021

Choose a reason for hiding this comment

akva2 commented Aug 4, 2021

joakim-hove commented Aug 5, 2021 • edited Loading

plgbrts commented Nov 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joakim-hove Nov 29, 2021 • edited Loading

Choose a reason for hiding this comment

joakim-hove commented Nov 29, 2021

joakim-hove commented Nov 29, 2021

joakim-hove commented Nov 30, 2021

goncalvesmachadoc commented Nov 30, 2021

plgbrts commented Dec 2, 2021

bska left a comment

Choose a reason for hiding this comment

plgbrts commented Dec 2, 2021

bska left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goncalvesmachadoc Dec 6, 2021 • edited by bska Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goncalvesmachadoc Dec 6, 2021 • edited by bska Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bska left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bska commented Dec 6, 2021

bska commented Dec 7, 2021

bska commented Dec 8, 2021

bska commented Dec 8, 2021

joakim-hove commented Aug 5, 2021 •

edited

Loading

joakim-hove Nov 29, 2021 •

edited

Loading

goncalvesmachadoc Dec 6, 2021 •

edited by bska

Loading

goncalvesmachadoc Dec 6, 2021 •

edited by bska

Loading