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

Remove assembly scratch.explicit_material_model and cleanup #783

Closed
wants to merge 2 commits into from

Conversation

gassmoeller
Copy link
Member

This PR depends on #775, which in turn depends on #769, so they should be reviewed and merged in order.

While working on #769 and #775 I noticed that we actually do not need to carry around a scratch.explicit_material_model any more, because the residual computation and the matrix assembly happens in separate loops over all cells since #425. Therefore we might as well reuse scratch.material_model, and just fill it with extrapolated values for the residual and current values for the assembly. At the moment we even fill explicit_material_model in the assembly, although we do not use it afterwards. Also this PR removes another peculiarity around the declaration of the material_model and heating_model_outputs for faces, which should be either completely in scratch (what I propose here), or completely in the advection face assembler function.

@gassmoeller
Copy link
Member Author

This PR only consists of the last commit above (for review)

@bangerth
Copy link
Contributor

bangerth commented Mar 7, 2016

When #769 is merged, can you rebase and ask for a review here? Or are the issues here independently reviewable from #769?

@gassmoeller gassmoeller force-pushed the cleanup_assembly branch 2 times, most recently from 0f69fdd to 8309a13 Compare March 8, 2016 16:06
@gassmoeller
Copy link
Member Author

This is also ready for review. This PR only consists of the last commit, the previous one belongs to #775.
This is really mostly cleaning up unused variables. The new local_assemble_advection_system function is much shorter and easier to understand. Should be faster as well.

@tjhei
Copy link
Member

tjhei commented Mar 9, 2016

d6b1297 looks very reasonable.

What is the reasoning behind the change of old_field_values from pointer to value? Not a big deal, just curious (you are doing an extra copy now).

@gassmoeller
Copy link
Member Author

Yes, I went for the one extra copy in get_artificial_viscosity to get rid of 1 or n_compositional_fields finite element evaluations in local_assemble_advection_system. Thinking about it, I probably could have avoided this one as well by having logic in local_assemble_advection_system that sets old_field_values to the proper field before evaluating and then only evaluates the current advection field and stores it in *scratch.old_field_values. But this way we also get rid of a lot of *scratch... in favor of more standard scratch.... Let me know if you prefer it the slightly more efficient but complicated way 😉

This was referenced Apr 5, 2016
@gassmoeller gassmoeller added this to the 1.4 milestone Apr 7, 2016
@tjhei
Copy link
Member

tjhei commented Apr 25, 2016

Regarding old_field_values, do we actually need to evaluate temperature and compositional fields and then conditionally copy one into old_field_values in get_artificial_viscosity?

I went through this again and I think it is ready to be merged. Let's wait on #775?

std::vector<double> *old_field_values;
std::vector<double> *old_old_field_values;
std::vector<double> old_field_values;
std::vector<double> old_old_field_values;
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, nice change.

tjhei added a commit to tjhei/aspect that referenced this pull request Apr 30, 2016
Conflicts:
	source/simulator/assembly.cc
@tjhei
Copy link
Member

tjhei commented Apr 30, 2016

It turns out we always need composition and temperature to evaluate material models...

@bangerth
Copy link
Contributor

@gassmoeller -- can you please rebase this PR to master?

@tjhei
Copy link
Member

tjhei commented Apr 30, 2016

no, I am already on it (and fixing a few things).

@tjhei
Copy link
Member

tjhei commented Apr 30, 2016

see #846

@tjhei tjhei closed this Apr 30, 2016
bangerth added a commit that referenced this pull request May 1, 2016
Merge #783 Remove assembly scratch.explicit_material_model and cleanup
@gassmoeller gassmoeller deleted the cleanup_assembly branch April 27, 2017 16:56
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.

None yet

3 participants