Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MatLib/MPL] Extend linear property #2649

Merged
merged 5 commits into from
Sep 6, 2019

Conversation

renchao-lu
Copy link
Contributor

@renchao-lu renchao-lu commented Sep 4, 2019

Following from Tom's suggestion, PR #2646 is split into two parts. As titled, this pull request extends for linear dependency on multiple variables.

  1. Feature description was added to the changelog
  2. Tests covering your feature were added?
  3. Any new feature or behavior change was documented?

std::get<double>(iv.reference_condition));
};

double linearized_ratio_to_reference_value =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
double linearized_ratio_to_reference_value =
double const linearized_ratio_to_reference_value =

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

_independent_variable.type)]) -
std::get<double>(_independent_variable.reference_condition)));
auto calculate_linearized_ratio =
[&variable_array](double initial_linearized_ratio, auto const& iv) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
[&variable_array](double initial_linearized_ratio, auto const& iv) {
[&variable_array](double const initial_linearized_ratio, auto const& iv) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

}


PropertyDataType LinearProperty::dValue(VariableArray const& /*variable_array*/,
Variable const primary_variable) const
{
return _independent_variable.type == primary_variable
auto independent_variable =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
auto independent_variable =
auto const independent_variable =

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@@ -42,6 +42,7 @@ using Tensor = std::array<double, 9>;
/// is missing, simply add it somewhere at the list, but above the last entry.
enum class Variable : int
{
concentration,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
concentration,

Does not belong into this PR, I guess...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved this commit into PR #2646

Copy link
Member

@endJunction endJunction left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few consts and drop concentration.
When/if the comments from the original PR are incorporated, ⏩

Copy link
Member

@wenqing wenqing left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Only one suggestion.

}


PropertyDataType LinearProperty::dValue(VariableArray const& /*variable_array*/,
Variable const primary_variable) const
{
return _independent_variable.type == primary_variable
auto independent_variable =
std::find_if(_independent_variables.begin(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you can use an array like

using IndependentVariableArray =
    std::array<IndependentVariable, static_cast<int>(Variable::number_of_variables)>;
IndependentVariableArray const _independent_variables; /// With initial value for specified entries
``
Then you can get the value by the entry index of `static_cast<int>( primary_variable). 

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wenqing If I understood you correctly, instead of a vector an array should be used with maximum possible variables.
Then we would need to store another list of integeres indicating in which variables the LinearProperty is multi-linear.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The additional list of integers wouldn't make the code better readable. And I don't think we would gain much speedup.

Another possibility would be to set the slope of the independent variables in which the property is not linear to zero - but this implies more computations. I would keep @renchao-lu implementation as it is.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By far I keep the implementation as it is.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.

@TomFischer TomFischer merged commit 4b54b4f into ufz:master Sep 6, 2019
@renchao-lu renchao-lu deleted the EnrichLinearProperty branch September 6, 2019 11:56
@endJunction
Copy link
Member

@renchao-lu Please add the description to the changelog. Also in other PR's.

@renchao-lu
Copy link
Contributor Author

@renchao-lu Please add the description to the changelog. Also in other PR's.

@endJunction Description of this pull request added half an hour ago. ^-^

@ogsbot
Copy link
Member

ogsbot commented Jun 19, 2020

OpenGeoSys development has been moved to GitLab.

See this pull request on GitLab.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants