Add support for directional relative permeabilities #714

hakonhagland · 2022-09-01T19:43:28Z

Depends on OPM/opm-material#532 and OPM/opm-common#3113

hakonhagland · 2022-09-01T19:50:25Z

jenkins build this opm-material=532 opm-common=3113 please

Adds support for directional relperms for the tpfa linearizer.

atgeirr

What I looked at seems good, I just have a few questions and minor requests.

atgeirr · 2022-09-15T06:27:27Z

opm/models/blackoil/blackoilintensivequantities.hh

@@ -588,6 +646,39 @@ private:
    friend BlackOilBrineIntensiveQuantities<TypeTag>;
    friend BlackOilMICPIntensiveQuantities<TypeTag>;

+    void updateRelperms(const Problem& problem, const MaterialLawParams& materialParams, unsigned globalSpaceIdx)


When it is spread across two functions, it is not easy to see that mobility_ and dirMob_ gets updated here. What about making this a static function and passing mobility_ and dirMob_ to it, that way it is obvious at the call site that those variables get updated?

Good idea! See update.

atgeirr · 2022-09-15T06:30:09Z

opm/models/discretization/ecfv/ecfvstencil.hh

+        /*!
+         * \brief Returns the direction of the face
+         */
+        FaceDir::DirEnum dirId() const


For my current work on boundary conditions, it would be very useful if dirId() returned the underlying int instead, as I need to distinguish between the XMin and XPlus cases. Could we make that change, and add a free function faceDirFromDirId() with the implementation below?

Yes, see update.

Thanks, I have added the code identically to my own branch too, so it should be easy to rebase one once the other is merged.

atgeirr · 2022-09-15T06:40:42Z

opm/models/blackoil/blackoilintensivequantities.hh

@@ -597,6 +688,7 @@ private:
    Evaluation porosity_;
    Evaluation rockCompTransMultiplier_;
    std::array<Evaluation,numPhases> mobility_;
+    std::shared_ptr<DirectionalMobility> dirMob_;


I assume this is a shared_ptr and not a unique_ptr to keep the class copyable. But I think that changes the semantics of the class in a nontrivial way, as we will then not get a fully independent copy anymore. I am not set against it, but have you looked at the places where IQs are copied and checked that this is ok?

If we want to preserve deep copy semantics with a smart pointer it is possible, the Thor library has (or at least used to have) a CopiedPtr template that implemented it: https://bromeon.ch/libraries/thor/tutorials/v1.1/smartptr.html

Not sure that is necessary though.

Yes! That was to make the IQs copyable. I am not sure if there is an easy way to find all the places where the copy constructor and assignment operator for the IQ are used. I checked some files and looked at the compiler error messages and found one place in fvbaseelmentcontext.hh where they are copied: see updateIntensitiveQuantities_() at line 562 and one place where the IQs are reassigned, see updateStencil() at line 140. There are probably other places?

In the updated PR, I changed the shared_ptr to unique_ptr and added a copy constructor and an assignment operator to the BlackOilIntensiveQuantities class.

I think it is not a good solution to add explicitly coded copy constructors and assignment operators. It adds to the maintenance overhead significantly. I am also not sure it is actually correct as implemented here in the copy constructor, it fails to copy most members, this just demonstrates the point that it is hard to maintain.

I think using a copyable smart pointer is the best solution, keeping the copy construction and assigment defaulted. I may have come across as skeptical of the shared_ptr, but I think it can be an acceptable solution, as long as we have considered the consequences. A smart pointer with copy semantics is also a fine solution I think, but requires us to create or adopt such a template class.

It adds to the maintenance overhead significantly

@atgeirr Yes, I agree.

A smart pointer with copy semantics is also a fine solution I think, but requires us to create or adopt such a template class.

I'll give the "copyable pointer" a try first. In the added commit I try to implement a minimal functionality for a copyable pointer.

To make each copy of the IQ unique, we change dirMob_ to be a shared_ptr instead of a unique_ptr.

To aid readability, we make updateRelperms() a static function.

Use a copyable unique pointer instead of writing custom copy constructor and assignment operator.

atgeirr · 2022-09-19T08:09:54Z

opm/models/blackoil/blackoilintensivequantities.hh

+            return ptr_ ? true : false;
+        }
+        // get a reference to the stored value
+        T& get() const {return *ptr_;}


To be consistent with the unique_ptr interface, it should return the raw pointer instead (return ptr_.get()).

atgeirr · 2022-09-19T08:11:31Z

opm/models/blackoil/blackoilintensivequantities.hh

+        CopyablePtr() : ptr_(nullptr) {}
+        CopyablePtr(const CopyablePtr& other) {
+            if (other) { // other does not contain a nullptr
+                ptr_ = std::make_unique<T>(other.get());


With get() returning a pointer (see below), this should change to take *this.

atgeirr · 2022-09-19T08:13:11Z

opm/models/blackoil/blackoilintensivequantities.hh

+            if (other) { // other does not contain a nullptr
+                ptr_ = std::make_unique<T>(other.get());
+            }
+            // else {ptr_ = nullptr;} // this is the default construction value


The else branch must be implemented to kill the current pointer. Copying from an empty smart pointer should leave the current object as a similarly empty one, just as in the assignment operator.

I thought since we are constructing a new object there will be no current object? Then, the default constructor will apply to the unique_ptr which will initialize it with a nullptr. Anyway, for clarity I have updated the code to use the else branch.

You are correct in that there is no current object, my mistake! Maybe I thought it was a move constructor or something, not sure. But the default constructor is not invoked automatically, so you should always ensure all members are initialized.

atgeirr · 2022-09-19T08:15:54Z

opm/models/blackoil/blackoilintensivequantities.hh

+    // of the unique_ptr each time we copy construct or assign to it from another BlackOilIntensiveQuantites.
+    // (On the other hand, if a copy could share the ptr with the original, a shared_ptr could be used instead and the
+    // wrapper would not be needed)
+    template <class T>


I would add one more warning to this class template comment: You should never use it polymorphically. (That would require a virtual clone() method etc.) It will only ever copy the static class type of the pointed to class.

Also, it should be put in a separate file, and have some unit tests.

Good idea, I have moved the class to opm-common, see OPM/opm-common#3113 and added some tests..

This is more in line with how std::unique_ptr works.

For clarity, explicitly default construct the unique_ptr in the copy constructor

Moved the CopyablePtr template class to opm-common, see file opm/utility/CopyablePtr.hpp in opm-common

Try to fix jenkins build error: no type named EclMaterialLawManager

Define a method materialLawManagerPtr() that returns a nullpointer to EclMaterialLawManager, but that can be overridden in derived classes e.g. EclProblem

bska · 2022-09-26T08:08:39Z

This PR appears to break the post-commit build. Please fix or revert.

totto82 · 2022-09-26T08:26:02Z

Strange, it was green. Let me first double-check locally. Could there be an issue with the ordering and timing of when this and the PR in opm-material was merged?

hakonhagland · 2022-09-26T08:26:59Z

This PR appears to break the post-commit build

I see in the jenkins log:

[5/119] Building CXX object CMakeFiles/reservoir_blackoil_vcfv.dir/tests/reservoir_blackoil_vcfv.cc.o
FAILED: CMakeFiles/reservoir_blackoil_vcfv.dir/tests/reservoir_blackoil_vcfv.cc.o

But locally on my machine the test reservoir_blackoil_vcfv in opm-models seems not to be built. Any ideas how I can build and test this locally?

bska · 2022-09-26T08:43:10Z

locally on my machine the test reservoir_blackoil_vcfv in opm-models seems not to be built. Any ideas how I can build and test this locally?

Configure all your module builds with the

-DBUILD_TESTING:BOOL=True

cmake option.

bska · 2022-09-26T08:48:43Z

Could there be an issue with the ordering and timing of when this and the PR in opm-material was merged?

I suppose, but I get the same result locally when I include BUILD_TESTING in my configuration options.

hakonhagland · 2022-09-27T04:59:43Z

Configure all your module builds with the

-DBUILD_TESTING:BOOL=True

@bska Thanks for the information!

This PR appears to break the post-commit build. Please fix or revert.

This has been addressed in #727. Thanks to @totto82 for ideas on how to solve this.

hakonhagland mentioned this pull request Sep 1, 2022

Add support for directional relative permeabilities OPM/opm-simulators#4048

Merged

hakonhagland added 4 commits September 14, 2022 15:34

Add support for KRNUM

a2eb477

Only store directional mobilities if needed

edb2d08

Fix typo

6827d6a

Add more support for directional relperms

78092a2

Adds support for directional relperms for the tpfa linearizer.

hakonhagland force-pushed the krnum branch from 933d61f to 78092a2 Compare September 14, 2022 21:53

atgeirr requested changes Sep 15, 2022

View reviewed changes

hakonhagland added 4 commits September 16, 2022 09:17

Use unique_ptr for dirMob_ instead of shared_ptr

64aff2b

To make each copy of the IQ unique, we change dirMob_ to be a shared_ptr instead of a unique_ptr.

Make updateRelperms() a static function

fafe69d

To aid readability, we make updateRelperms() a static function.

Rename dirId() to faceDirFromDirId()

6b25347

Use a copyable unique pointer

30c9455

Use a copyable unique pointer instead of writing custom copy constructor and assignment operator.

atgeirr reviewed Sep 19, 2022

View reviewed changes

hakonhagland added 7 commits September 19, 2022 12:03

Add missing include file

74b9b64

Make get() return a pointer instead of a reference

6f50af8

This is more in line with how std::unique_ptr works.

Explicitly initialize member in copy constructor

d8f7018

For clarity, explicitly default construct the unique_ptr in the copy constructor

Move the CopyablePtr class to a separate file

01f9a9c

Moved the CopyablePtr template class to opm-common, see file opm/utility/CopyablePtr.hpp in opm-common

Include missing header

555abc2

Try to fix: no type named EclMaterialLawManager

de0b32c

Try to fix jenkins build error: no type named EclMaterialLawManager

Return a nullptr to the EclMaterialLawManager

7eb22f0

Define a method materialLawManagerPtr() that returns a nullpointer to EclMaterialLawManager, but that can be overridden in derived classes e.g. EclProblem

hakonhagland mentioned this pull request Sep 22, 2022

Add support for directional relative permeabilties OPM/opm-common#3113

Merged

atgeirr mentioned this pull request Sep 24, 2022

Boundary condition support for TpfaLinearizer #718

Merged

totto82 merged commit 4cc8da1 into OPM:master Sep 26, 2022

bska mentioned this pull request Sep 26, 2022

Internalise WRFTPLT Segment Option OPM/opm-common#3155

Merged

This was referenced Sep 26, 2022

Move updateRelperms() to the problem class OPM/opm-simulators#4132

Merged

Move updateRelperms() to the problem class #727

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for directional relative permeabilities #714

Add support for directional relative permeabilities #714

hakonhagland commented Sep 1, 2022 •

edited

Loading

hakonhagland commented Sep 1, 2022

atgeirr left a comment

atgeirr Sep 15, 2022

hakonhagland Sep 16, 2022

atgeirr Sep 15, 2022

hakonhagland Sep 16, 2022

atgeirr Sep 16, 2022

atgeirr Sep 15, 2022

hakonhagland Sep 16, 2022 •

edited

Loading

atgeirr Sep 16, 2022

hakonhagland Sep 18, 2022

atgeirr Sep 19, 2022

hakonhagland Sep 19, 2022

atgeirr Sep 19, 2022

atgeirr Sep 19, 2022

hakonhagland Sep 19, 2022

atgeirr Sep 20, 2022

atgeirr Sep 19, 2022

hakonhagland Sep 19, 2022

bska commented Sep 26, 2022

totto82 commented Sep 26, 2022

hakonhagland commented Sep 26, 2022 •

edited by bska

Loading

bska commented Sep 26, 2022

bska commented Sep 26, 2022

hakonhagland commented Sep 27, 2022

Add support for directional relative permeabilities #714

Add support for directional relative permeabilities #714

Conversation

hakonhagland commented Sep 1, 2022 • edited Loading

hakonhagland commented Sep 1, 2022

atgeirr left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hakonhagland Sep 16, 2022 • edited 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bska commented Sep 26, 2022

totto82 commented Sep 26, 2022

hakonhagland commented Sep 26, 2022 • edited by bska Loading

bska commented Sep 26, 2022

bska commented Sep 26, 2022

hakonhagland commented Sep 27, 2022

hakonhagland commented Sep 1, 2022 •

edited

Loading

hakonhagland Sep 16, 2022 •

edited

Loading

hakonhagland commented Sep 26, 2022 •

edited by bska

Loading