Scalar dynamic evaluation #4673
Conversation
GitPaean
added
the
manual:irrelevant
|
jenkins build this please |
1 similar comment
|
jenkins build this please |
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for scalar dynamic evaluation by implementing constant evaluation handling in the DynamicEvaluation class. The changes allow operations between evaluations with different derivative sizes, where one can be a constant (size 0).
- Adds a new constructor for creating constant evaluations with zero derivatives
- Implements logic to handle operations between evaluations of different sizes when one is constant
- Updates createConstant method to return a proper constant evaluation instead of throwing
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 12 comments.
| File | Description |
|---|---|
| opm/material/densead/DynamicEvaluation.hpp | Adds constant evaluation support and mixed-size operation handling |
| opm/material/common/FastSmallVector.hpp | Adds expandSize method to support dynamic resizing of evaluation storage |
| jenkins/pre-build.sh | Comments out code generation script (temporary test change) |
|
jenkins build this please |
GitPaean
force-pushed
the
scalar_dynamic_evaluation
branch
2 times, most recently
from
558307a to
64de5a2
Compare
|
jenkins build this please |
|
It looks like some rounding error or other mistakes jumping in caused regression. Working on recovering it. |
|
jenkins build this please |
| } | ||
| } | ||
|
|
||
| void expandSize(size_t numElem) |
There was a problem hiding this comment.
I suggest a more general resize() method, avoiding the assert() below and letting it both grow and shrink. Like std::vector, I suggest that when shrinking we do not actually deallocate, or switch back to the smallBuf_ if size decreases to <= N, but just stay with the representation we have (that would also preserve iterators when shrinking).
There was a problem hiding this comment.
If we do not switch back from std::vector<ValueType> data_ to std::array<ValueType, N> smallBuf, when size_ decreases to <=N, there will be other functions need to be changed for consistency.
Let me done update it first.
| assert(numElem >= size_); | ||
| if (size_ <= N) { | ||
| if (numElem > N) { | ||
| data_.resize(numElem, 0); |
There was a problem hiding this comment.
There should be no second argument. 0 will work for numbers but not for strings for example, or vectors.
Also on line 211.
| if (size_ <= N) { | ||
| if (numElem > N) { | ||
| data_.resize(numElem, 0); | ||
| data_.assign(smallBuf_.begin(), smallBuf_.begin() + size_); |
There was a problem hiding this comment.
This call will make the contents of data_ equal to the current contents of the object, making the resize() above pointless. I.e. if N is 10 and the object has size_ == 7, then we call this function with numElem == 12, we will end up with a data_ vector of size 7!
I suggest using a single resize() and instead of assign() just copy() the existing elements to the start of data_.
There was a problem hiding this comment.
assign indeed changes the size of the vector. thanks for pointing it out.
GitPaean
force-pushed
the
scalar_dynamic_evaluation
branch
from
935b81b to
1b92264
Compare
|
jenkins build this please |
GitPaean
force-pushed
the
scalar_dynamic_evaluation
branch
from
1b92264 to
70c0f1e
Compare
|
jenkins build this please |
GitPaean
force-pushed
the
scalar_dynamic_evaluation
branch
from
70c0f1e to
2e9da52
Compare
|
jenkins build this please |
|
With some discussion, it looks like this is an helpful/useful things to do. I am removing the @atgeirr , I have updated the PR, please have a look. I will migrate the implementation to the |
|
I have updated the genEvalSpecializations.py to generate the changes introduced in this PR and marking the PR is ready for review. I updated the description for this PR and mark it as ready for review. |
GitPaean
force-pushed
the
scalar_dynamic_evaluation
branch
from
8e2c080 to
0796fd1
Compare
Such DyanmicEvaluation will have size() of zero.
it is renamed to a more general resize and the function is revised accordingly.
|
jenkins build this please |
|
I will make an accompanying PR in the opm-simulators to test how it works. Currently only test it with a development PR while not tested the usage in the master branch, so I am converting this PR to be draft again. |
The main purpose of this PR is to be able to create a DynamicEvaluation directly/implicitly from a constant, like 0.0 or 1.0 or 0.5, without specifying the number of the derivatives.
For example, for the following code,
EvalWell cq_s_zfrac_effective{this->primary_variables_.numWellEq() + Indices::numEq, 0.0};now we can do
EvalWell cq_s_zfrac_effective{0.0};To do this, we represent constants with DynamicEvaluation without derivatives. And the operators are updated to incorporate this change.
The main motivation was that we have a lot of material related calculation using implicit conversion from scalar values. When we tried to do more material related calculation (similar to flash calculation for black oil) inside the wellbore in Standard Well, it appeared that lacking of implicit conversion caused issues. We can (tried to) update the material calculations to handle DyanmiceEvaluation, while it will affect many files and the approach introduced this PR is the more confined manner.