Closed trinemykk closed 2 years ago
@totto82 and @atgeirr please take a look
jenkins build this please
jenkins build this please
The obvious question is: How should we go about reviewing and merging this? Going through all the code (~1200 loc for PTFlash, plus ~1800 loc in other classes) in detail is a monumental task. On the other hand, it seems to me that most of this code has already been seen by and/or worked on by multiple persons.
So: what aspects of the code should be looked at, tested or reviewed before we press the green button? What classes or functions can be considered “already reviewed”? Do the two tests cover all the code?
benchmark please
Benchmark result overview:
Test | Configuration | Relative |
---|---|---|
opm-git | OPM Benchmark: flow_mpi_extra - Threads: 1 | 1.001 |
opm-git | OPM Benchmark: flow_mpi_extra - Threads: 8 | 1.004 |
opm-git | OPM Benchmark: flow_mpi_norne - Threads: 1 | 1.004 |
opm-git | OPM Benchmark: flow_mpi_norne - Threads: 8 | 1.001 |
opm-git | OPM Benchmark: flow_mpi_norne_4c_msw - Threads: 1 | 1.002 |
opm-git | OPM Benchmark: flow_mpi_norne_4c_msw - Threads: 8 | 1.006 |
View result details @ https://www.ytelses.com/opm/?page=result&id=1693
The obvious question is: How should we go about reviewing and merging this? Going through all the code (~1200 loc for PTFlash, plus ~1800 loc in other classes) in detail is a monumental task. On the other hand, it seems to me that most of this code has already been seen by and/or worked on by multiple persons.
So: what aspects of the code should be looked at, tested or reviewed before we press the green button? What classes or functions can be considered “already reviewed”? Do the two tests cover all the code?
@atgeirr , my view on this is the following:
0) the main contribution is the file opm/material/constraintsolvers/PTFlash.hpp, this is the actual flash where the whole procedure is done (including stability test, rachford rice, ssi, newton, handling of the derivatives..) - the results from the substeps (and the end result) is manually checked against the test in [git@github.com:moyner/MultiComponentFlash.jl.git] , for two fluidsystems opm/material/fluidsystems/ThreeComponentFluidSystem.hh and opm/material/fluidsystems/Co2BrineFluidSystem.hh through the tests _tests/test_co2brineptflash.cpp and _tests/test_threecomponentsptflash.cpp , so the functionality of this code can in my opinion be merged directly (but the technical coding can definitely be improved, but I am not sure this is important at this point) A necessary complementary file is opm/material/fluidsystems/PTFlashParameterCache.hpp, that is very similar to the other ParameterCache-files found in the directory
1) the files that are modified with minor things (and can be merged directly) are: CMakeLists.txt, opm/material/common/MathToolbox.hpp, opm/material/components/Brine.hpp, opm/material/components/Component.hpp, opm/material/components/H2O.hpp, opm/material/components/N2.hpp, opm/material/components/N2.hpp, opm/material/components/SimpleCO2.hpp, opm/material/components/TabulatedComponent.hpp, opm/material/components/iapws/Common.hpp, opm/material/densead/Math.hpp, opm/material/fluidsystems/BaseFluidSystem.hpp, opm/material/fluidsystems/Spe5ParameterCache.hpp, tests/checkComponent.hpp, opm/material/fluidstates/FluidStateCompositionModules.hpp
2) the files that are modified for the purpose of getting the flash to work are: opm/material/eos/PengRobinson.hpp, opm/material/eos/PengRobinsonMixture.hpp, opm/material/eos/PengRobinsonParamsMixture.hpp, since the former version of these files didn't produce the wanted roots of the cubic polynomial solved through the stability test and different derivatives on one of the equations (compared to the test from Moyner in 0)), it is safe to merge this directly. The former version of these files has never been tested and verified against other simulators like we have done now.
3) the files we have added as extra functionality needed are: the viscositymodels: _opm/material/viscositymodels/LBC.hp_p, opm/material/viscositymodels/LBCmodified.hpp, these are "standard" methods found in the literature and the results produced are manually compared against viscosities produced by [git@github.com:moyner/MultiComponentFlash.jl.git], and this can in my opinion be merged when the Norwegian comments are fixed as @bska pointed out
4) the specific fluidsystems tested here needs the added components opm/material/components/C1.hpp, opm/material/components/C10.hpp, (nothing controversial here), and another component added since we had it here was opm/material/components/H2.hpp (not needed by the tests, but nice to have in place), can be merged directly
5) In the file opm/material/common/PolynomialUtils.hpp a more standard cubic root solver is added cubicRoots(), this one produces the same roots as the compared code - so this functionality should also be safe to merge
Summary:
Just a note. Besides the single phase situation, we should also test all the three flash solution methods,
ssi , newton, ssi+newton
I can help some.
Thanks @trinemykk for your extensive explanation. After reading this I agree with you: The only modification of existing behaviour is for things (PengRobinson etc.) that were not working well, so it is fine to merge it from a behaviour point of view.
From a testing p.o.v. I think the additions discussed above (single-phase, and all three flash solution methods) sounds sufficient.
Finally, code quality: I agree that it is better to have something that works than to have nothing... While I have not looked at the code very much myself, knowing that it has been written in collaboration by at least two persons should ensure a sufficient level of quality.
jenkins build this please
jenkins build this please
jenkins build this please
jenkins build this please
Thank you for your effort, I will merge this now!
Added the isobaric-isothermal equilibrium (PT Flash) to constraint solvers, work done together with Svenn, Kai and Halvor. This is compared to implementations in MRST and Julia by Olav Moyner. Two tests, one standard three components and one with co2 and brine as two components are added. The more classical cubic root finder is implemented to find the needed roots, and the derivatives is now handled through the implicit function theorem. Some of the components have criticalVolume() and acentricFactor(), that is needed in this flash. Have also added opm/material/viscositymodels which currently contain two viscositymodels: the standard LBC from (Lohrenz, Bray and Clark 1964), and the modified LBC more specific for CO2rich mixtures (Lansangan, Taylor, Smith & Kovarik 1993). Thanks to Ove for the viscosity implementation.
PTFlash will be improved, so this can be looked as a draft PR.