Minor adjustments #4395
Minor adjustments #4395
Conversation
|
jenkins build this please |
1 similar comment
|
jenkins build this please |
opm/input/eclipse/Parser/Parser.cpp
Outdated
| if (this->pathMap.find(stringToFind) != this->pathMap.end()) { | ||
| std::string stringToReplace = this->pathMap.at(stringToFind); | ||
| replaceAll(path, pathKeywordPrefix + stringToFind, stringToReplace); |
There was a problem hiding this comment.
We should probably omit multiple searches.
| if (this->pathMap.find(stringToFind) != this->pathMap.end()) { | |
| std::string stringToReplace = this->pathMap.at(stringToFind); | |
| replaceAll(path, pathKeywordPrefix + stringToFind, stringToReplace); | |
| if (auto candidate = this->pathMap.find(stringToFind); candidate != this->pathMap.end()) { | |
| replaceAll(path, pathKeywordPrefix + stringToFind, *candidate); |
Would also be cool, if you add a test that triggers this.
There was a problem hiding this comment.
I have now improved the code based on your suggestion. I have also looked for a test file to trigger this error, but I did not find it.
The plan is to us a new version of opm-common soon. We aim for having as few patches to I will investigate to see if I can provide more info related to the bugfixes. |
|
Most of these issues are seen in older grid cases. These cases are not possible to share, as they are based on real field cases. I think this PR is ready for merge, please let me know if you need more info. |
There was a problem hiding this comment.
While the code changes are obviously small the semantic changes are considerable here. Especially the change to opm/io/eclipse/rst/state.cpp is a little too large for comfort. That one, in particular, opens the door to use cases which the overall facility is not set up to handle and which I don't think are appropriate for the overall logic here.
I realise that the needs of ResInsight are very different from the needs of the OPM Flow simulator's restart facility, and as such I'd really prefer if at least that part could be kept as an independent patch in the ResInsight repository.
Loading all arrays is useful in some workflows, but will cause a significant performance penalty for larger cases. The data will be loaded on demand when required in RestartFileView::getKeyword<T>()
magnesj
force-pushed
the
minor-adjustments
branch
from
7de7445 to
1b52437
Compare
|
I have updated this PR with a smaller subset of fixes. Please let me know if you need more info. |
|
jenkins build this please |
Several adjustments based on testing on regression test datasets in ResInsight.
One commit is related to performance, avoid loading data until requested.