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

[Core] Add distribute variable method to variable utils #7146

Merged
merged 15 commits into from
Jun 28, 2020

Conversation

sunethwarna
Copy link
Member

This adds a method to variable_utils to distribute variables(double, and array_3d) in elemental or condition data value container to nodal non-historical data value container with a specified weight. Tests are also added.

@sunethwarna sunethwarna requested a review from philbucher June 27, 2020 05:42
Copy link
Member

@rubenzorrilla rubenzorrilla left a comment

Choose a reason for hiding this comment

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

Some implementation comments. Other than this, I'm fine with the changes.

sunethwarna and others added 3 commits June 27, 2020 17:03
Co-authored-by: Rubén Zorrilla <rubenzorrillamartinez@hotmail.com>
Co-authored-by: Rubén Zorrilla <rubenzorrillamartinez@hotmail.com>
Co-authored-by: Rubén Zorrilla <rubenzorrillamartinez@hotmail.com>
Co-authored-by: Rubén Zorrilla <rubenzorrillamartinez@hotmail.com>
Copy link
Member

@rubenzorrilla rubenzorrilla left a comment

Choose a reason for hiding this comment

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

Nothing else from my side. Thanks @sunethwarna !!!

@sunethwarna sunethwarna merged commit f74a5a6 into master Jun 28, 2020
@sunethwarna sunethwarna deleted the core/variable_utils/add_distribute_variable_method branch June 28, 2020 07:13
@philbucher
Copy link
Member

guys please don't open PRs on Sat and merge on Sunday, esp for additions to the core

note to self: add to code of conduct

Comment on lines +516 to +519
const std::function<double(const Node<3>&)>& r_weight_method =
(IsInverseWeightProvided) ?
static_cast<std::function<double(const Node<3>&)>>([rWeightVariable](const Node<3>& rNode) -> double {return 1.0 / rNode.GetValue(rWeightVariable);}) :
static_cast<std::function<double(const Node<3>&)>>([rWeightVariable](const Node<3>& rNode) -> double {return rNode.GetValue(rWeightVariable);});
Copy link
Member

Choose a reason for hiding this comment

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

  1. I think using std function has some overhead, use with caution in performance critical parts
  2. There is no check for rNode.GetValue(rWeightVariable). If it is 0 then it will crash. This happens if this var was not allocate

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. I added the check in full debug only. Should I change it to release as well? I didnt do it considering the performance impact. (
    KRATOS_DEBUG_ERROR_IF(!r_node.Has(rWeightVariable))
    << "Non-historical nodal " << rWeightVariable.Name() << " at "
    << r_node << " is not initialized in " << rModelPart.Name()
    << ". Please initialize it first.";
    )
  2. As for the zeros, I can add a check in full debug again. Would it be sufficient?

@sunethwarna
Copy link
Member Author

@philbucher sorry, I wasn't thinking like that. I will keep this in mind in future :/

@sunethwarna sunethwarna restored the core/variable_utils/add_distribute_variable_method branch June 28, 2020 11:15
@sunethwarna sunethwarna deleted the core/variable_utils/add_distribute_variable_method branch September 9, 2020 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants