Correct suffix for solution tracers

[svenn-t]

Needed in (draft) PR: OPM/opm-simulators#5268.

[bska]

There is one more instance of the magic number 4 in related to tracers in Summary.cpp. This constant may or may not be important for this work–I don't know off hand–but we should at least add a comment for why we don't need to adjust it if it is not.

Other than that, this looks good to me.

[bska]

You could make istart be const by writing this as

const auto istart = 4 + ((normKw[4] == 'F') || (normKw[4] == 'S'));

but I suppose it's in the eye of the beholder whether or not that's a simplification.

[svenn-t]

I like the simplification so I changed the code. Thanks!

[svenn-t]

There is one more instance of the magic number 4 in related to tracers in Summary.cpp. This constant may or may not be important for this work–I don't know off hand–but we should at least add a comment for why we don't need to adjust it if it is not.

Other than that, this looks good to me.

The magic number 4 in ratetracer() actually works because for free and solution tracers the summary vectors are stored with a "F" or "S" prefix in the tracer_name. So for example the summary vector WTPRFX11 is stored with tracer_name = FX11 to get the free part of a tracer with actual name X11.

[akva2]

in that case something like

constexpr auto prefix_len = std::strlen("WTPR");

is a lot more expressive than the magic number.

[svenn-t]

in that case something like

constexpr auto prefix_len = std::strlen("WTPR");

is a lot more expressive than the magic number.

I agree, magic numbers are not the ideal. The ratetracer() function is used for all well-related tracer keywords (WTPC, FTPT, etc), so checking the length of a specific keyword doesn't work here, at least. But maybe it can be defined somewhere else, in the way you suggest, and put into the args input? I'm not so familiar with how everything works in the Summary.cpp file.

[akva2]

right, if it applies to more keywords something ala

constexpr auto prefix_len = 4; // 'WTPRxxx, FTPTxxx, ...

conveys the point just as well.

[svenn-t]

Agreed, I have updated the code. Thanks!

[totto82]

jenkins build this opm-simulators=5268 please

[totto82]

jenkins build this opm-simulators=5268 please

[totto82]

jenkins build this opm-simulators=5268 please

[bska]

benchmark please opm-simulators=5268

[ytelses]

Benchmark result overview:

Test	Configuration	Relative
opm-git	OPM Benchmark: drogon - Threads: 1	1.007
opm-git	OPM Benchmark: drogon - Threads: 8	0.995
opm-git	OPM Benchmark: punqs3 - Threads: 1	1.013
opm-git	OPM Benchmark: punqs3 - Threads: 8	0.989
opm-git	OPM Benchmark: smeaheia - Threads: 1	1.014
opm-git	OPM Benchmark: smeaheia - Threads: 8	1.006
opm-git	OPM Benchmark: spe10_model_1 - Threads: 1	1.004
opm-git	OPM Benchmark: spe10_model_1 - Threads: 8	1
opm-git	OPM Benchmark: flow_mpi_extra - Threads: 1	0.997
opm-git	OPM Benchmark: flow_mpi_extra - Threads: 8	1.001
opm-git	OPM Benchmark: flow_mpi_norne - Threads: 1	0.987
opm-git	OPM Benchmark: flow_mpi_norne - Threads: 8	1.001
opm-git	OPM Benchmark: flow_mpi_norne_4c_msw - Threads: 1	1.013
opm-git	OPM Benchmark: flow_mpi_norne_4c_msw - Threads: 8	0.993

• Speed-up = Total time master / Total time pull request. Above 1.0 is an improvement. *

View result details @ https://www.ytelses.com/opm/?page=result&id=2505

[bska]

Is this PR itself independent of the downstream PR OPM/opm-simulators#5268, or must the two be reviewed and merged in concert?

[svenn-t]

Is this PR itself independent of the downstream PR OPM/opm-simulators#5268, or must the two be reviewed and merged in concert?

I have not tried to compile changes in this PR with the master version of opm-simulators, so I don't know if any of the tests will fail or other unexpected errors will occur. Should mostly be extensions of summary file output in this PR so it will perhaps be fine to treat this PR independently.

[bska]

Is this PR itself independent of the downstream PR OPM/opm-simulators#5268, or must the two be reviewed and merged in concert?

I have not tried to compile changes in this PR with the master version of opm-simulators, so I don't know if any of the tests will fail or other unexpected errors will occur. Should mostly be extensions of summary file output in this PR so it will perhaps be fine to treat this PR independently.

Understood. In that case I'll try a separate build check just for this PR and review/merge if that check goes through.

[bska]

jenkins build this please

[bska]

Thanks a lot. This looks good to me now and I'll merge into master.