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

Use constructor and unroll function for tensors #5641

Merged
merged 1 commit into from
May 30, 2024

Conversation

gassmoeller
Copy link
Member

This PR is similar to #5640, except it only uses the existing unroll functions and constructors of the Tensor class to simplify the creation and unrolling of tensors from and into compositional fields.

Comment on lines 32 to 34
const ArrayView<double> strain_increment_values(&out.reaction_terms[q][0],
Tensor<2,dim>::n_independent_components);
strain_increment.unroll(strain_increment_values.begin(), strain_increment_values.end());
Copy link
Member

Choose a reason for hiding this comment

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

wouldn't be simpler to do

Suggested change
const ArrayView<double> strain_increment_values(&out.reaction_terms[q][0],
Tensor<2,dim>::n_independent_components);
strain_increment.unroll(strain_increment_values.begin(), strain_increment_values.end());
strain_increment.unroll(strain_increment.unroll(&out.reaction_terms[q][0], &out.reaction_terms[q][0]+Tensor<2,dim>::n_independent_components);

?

Copy link
Member

Choose a reason for hiding this comment

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

I am also not sure why unroll actually needs an end iterator. The interface would be nicer if it would take a single iterator...

Comment on lines 91 to 93
for (unsigned int i = 0; i < Tensor<2,dim>::n_independent_components ; ++i)
out.reaction_terms[q][i] = strain_increment[Tensor<2,dim>::unrolled_to_component_indices(i)];
const ArrayView<double> strain_increment_values(&out.reaction_terms[q][0],
Tensor<2,dim>::n_independent_components);
strain_increment.unroll(strain_increment_values.begin(), strain_increment_values.end());
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this is easier to read. The issue with unroll() is that it isn't clear which direction the operation goes. Is the tensor putting itself into an array, or is it reading new values from the array? This was clear in the original code.

@gassmoeller
Copy link
Member Author

I updated the PR according to the discussions in #5640. I cannot change the name of unroll here until I made the change in deal.II.

@bangerth
Copy link
Contributor

Something is still broken -- the only thing that works is the spell checker :-)

@gassmoeller
Copy link
Member Author

I guess that shows how much I tested this after the change :-). Fixed by deleting an unnecessary closing parenthesis.

@bangerth
Copy link
Contributor

It's still not quite right:

/__w/aspect/aspect/tests/simple_shear_output_the_mobility.cc: In member function 'virtual void aspect::MaterialModel::FiniteStrain<dim>::evaluate(const aspect::MaterialModel::MaterialModelInputs<dim>&, aspect::MaterialModel::MaterialModelOutputs<dim>&) const':
/__w/aspect/aspect/tests/simple_shear_output_the_mobility.cc:85:35: error: redeclaration of 'const dealii::Tensor<2, dim, double> strain'
   85 |               const Tensor<2,dim> strain(strain_values)
      |                                   ^~~~~~
/__w/aspect/aspect/tests/simple_shear_output_the_mobility.cc:83:35: note: 'const dealii::Tensor<2, dim, double> strain' previously declared here
   83 |               const Tensor<2,dim> strain(make_array_view(&in.composition[q][0],
      |                                   ^~~~~~
/__w/aspect/aspect/tests/simple_shear_output_the_mobility.cc:85:42: error: 'strain_values' was not declared in this scope
   85 |               const Tensor<2,dim> strain(strain_values)
      |                                          ^~~~~~~~~~~~~

@gassmoeller
Copy link
Member Author

Ok, third time's the charm. Seems to be working now.

@bangerth bangerth merged commit d22bb44 into geodynamics:main May 30, 2024
7 of 8 checks passed
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.

3 participants