OPM
Updating MICP
The MICP implementation is updated to use the new lineariser, support dispersion, diffusion, sources, bcs, and summary variables. In addition, now to set the parameters the keyword BIOFPARA (which can be set per SATNUM region) is used instead of MICPPARA, since it is planned to reuse keywords when we add biofilm effects in H2STORE simulations (e.g., now also PERMFACT is used). The two decks in opm-tests are also updated.
These PRs will required an updated to the manual, which I will create later.
No more warnings and the only test failing is flow+MICP as expected. Both Prs are set now as ready for review, and the plan is later to update the reference solution for flow+MICP with the new keywords (i.e., BIOFPARA, PERMFACT, DIFFMICP, and SUMMARY ones).
Thanks @akva2 for your comments. Still the only test failing is flow+MICP, which needs an update in the data solution. To update the data via jenkins, then I will wait until both PRs are approved. I also asked @totto82 for a review and he said he could do it next week. If anyone else would have comments/corrections/improvements, please let me know.
Still the only test failing is flow+MICP, which needs an update in the data solution.
Just to be clear, the symptoms in the currently failing do not appear to be just an out-of-date reference solution. Here are the final few lines of the transcript:
24 Reading DENSITY in /var/lib/jenkins/workspace/opm-common-PR-builder/deps/opm-tests/micp/MICP.DATA line 73 25 Reading PVTW in /var/lib/jenkins/workspace/opm-common-PR-builder/deps/opm-tests/micp/MICP.DATA line 76 Error: Unknown keyword: MICPPARA Error: Unrecoverable errors while loading input: Problem with keyword MICPPARA In /var/lib/jenkins/workspace/opm-common-PR-builder/deps/opm-tests/micp/MICP.DATA line 79 Unknown keyword: MICPPARA
Is this the expected failure mode?
Is this the expected failure mode?
It is, now different keywords are used (as described at the top of this PR), in addition to changes on how the model parts are implemented (e.g., now the poro-perm relationship effects are included in the transMult, before they were on the mobility). Then I have looked locally at the results of the new implementation with the new added summary keywords for the mass computations and it seems there is no issues. The results using these PRs and current master compare well qualitatively, and the quantitively differences are expected.
Is this the expected failure mode?
It is,
Okay, so that basically means we can't merge this as is. We'd either need to disable the test, merge this, update the test input, and then re-enable the test with (possibly) new reference solutions. Alternatively, we'd need to merge this alongside the updated test input, the PR of which would possibly be amended with a set of updated reference solutions.
To be clear: I'm not merging anything which breaks the build and which cannot be automatically fixed.
Alternatively, we'd need to merge this alongside the updated test input, the PR of which would possibly be amended with a set of updated reference solutions.
Yes, that is what I wanted to mean, sorry for not being clear.
The first execution of jenkins build this opm-simulators=6105 opm-tests=1311 update_data please worked fine. After inspecting to the results, then I notice --enable-opm-rst-file=1 was not added in regressionTests.cmake. Then I added it but after couple of attempts to execute jenkins build this opm-simulators=6105 opm-tests=1311 update_data please the new data were not generated, then I just gave up and remove the --enable-opm-rst-file=1 flag.
Now all tests are successful.
I don't have any further comments on this PR beyond what has already been commented on and addressed. Thanks for your contribution to improving the MICP model.
The test failure corresponds to mpi.compareECLFiles_flowexp_comp+1D_COMP, which it will be temporally disabled in https://github.com/OPM/opm-simulators/pull/6141, then I think the MICP PR'S should be fine to merge @totto82