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

reduce storage for matrix-free gmg #3291

Merged
merged 2 commits into from Nov 15, 2019

Conversation

@tcclevenger
Copy link
Contributor

tcclevenger commented Nov 12, 2019

Reduce the storage of the matrix-free GMG Stokes setup by only storing one viscosity value for each cell instead of one viscosity value for each quadrature point. This was left over from old code where I was querying viscosity values from some functional representation on each level, not using averaging. When we update the code in the future to not requiring averaging, this may look different. In the meantime, this greatly reduces storage for the case of cell-wise coefficient averaging.

I did not create any new tests since this is not a new feature, and all tests currently using GMG should pass.

For all pull requests:

include/aspect/stokes_matrix_free.h Outdated Show resolved Hide resolved
@@ -191,7 +191,7 @@ namespace aspect
/**
* Table which stores the viscosity on each quadrature point.
*/
Table<2, VectorizedArray<number> > one_over_viscosity;
Table<1, VectorizedArray<number> > one_over_viscosity;

This comment has been minimized.

Copy link
@tjhei

tjhei Nov 13, 2019

Member

are these tables in the three operators the same? if yes, you could only compute and store it once and only keep a reference here.

This comment has been minimized.

Copy link
@tcclevenger

tcclevenger Nov 13, 2019

Author Contributor

The issue is that the cells withing the matrix-free object can be iterated through in a different ordering depending on the matrix-free operator and so the indexing could change for each table.

This comment has been minimized.

Copy link
@tjhei

tjhei Nov 13, 2019

Member

I guess we are now talking only about a couple of MB, so I am okay with it. I would be surprised if the order is different though?

This comment has been minimized.

Copy link
@tcclevenger

tcclevenger Nov 13, 2019

Author Contributor

You are right, it is different for DG finite elements and continuous finite elements, but this isn't an issue for us since all operators are based on continuous elements.

I could add a function to each operator that sets a viscosity table?

This comment has been minimized.

Copy link
@tcclevenger

tcclevenger Nov 13, 2019

Author Contributor

I say we don't worry right now with the reduction of the viscosity tables into 1. This whole structure will look different when we add in the higher order projection.

This comment has been minimized.

Copy link
@tjhei

tjhei Nov 13, 2019

Member

ok, let's do that later.

source/simulator/stokes_matrix_free.cc Outdated Show resolved Hide resolved
source/simulator/stokes_matrix_free.cc Outdated Show resolved Hide resolved
source/simulator/stokes_matrix_free.cc Outdated Show resolved Hide resolved
source/simulator/stokes_matrix_free.cc Outdated Show resolved Hide resolved
@tjhei tjhei added the ready to test label Nov 13, 2019
Copy link
Contributor Author

tcclevenger left a comment

I've made changes based on your comments. I have responded on two of them.

@tjhei
tjhei approved these changes Nov 13, 2019
@tjhei

This comment has been minimized.

Copy link
Member

tjhei commented Nov 13, 2019

I am okay with the changes, but I let others have a chance at looking at it before merging.

@gassmoeller

This comment has been minimized.

Copy link
Contributor

gassmoeller commented Nov 15, 2019

I am fine with the changes as well. Could you give a quick update on the state of using variable viscosities within the cells? This is mostly so that I know how to plan future work, and what to say if I am asked about it.

@gassmoeller gassmoeller merged commit b09cb2c into geodynamics:master Nov 15, 2019
2 checks passed
2 checks passed
continuous-integration/jenkins/pr-merge This commit looks good
Details
jenkins.tjhei.info/pr-head This commit looks good
Details
@tcclevenger

This comment has been minimized.

Copy link
Contributor Author

tcclevenger commented Nov 21, 2019

Thanks @gassmoeller!

I just finished defending my thesis. Now I will have much more time to work on the coefficient averaging. The new averaging is the top priority now. Timo and I will discuss this soon and I'll be able to give you a better answer, but I would like to have significant progress on it by the middle of December.

The idea is still the same: Cellwise averaging where the viscosity is discontinuous, Q2 or DGQ2 interpolation where the viscosity is continuous. For all the other coefficients (density etc), we never need to touch them so I imagine leaving them as quadrature point representations is ok?

@tcclevenger tcclevenger deleted the tcclevenger:fix_viscosity_tables_gmg branch Nov 21, 2019
@gassmoeller

This comment has been minimized.

Copy link
Contributor

gassmoeller commented Nov 21, 2019

Awesome, congratulations on your defense! You did some great work with the GMG!

Yeah the plans sound perfect. Let me know when you want input or model problems to toy around with.

@tjhei

This comment has been minimized.

Copy link
Member

tjhei commented Nov 21, 2019

I would like to add that viscosity averaging is high on our list, but there is other important stuff that will keep us busy for the rest of the year.
A realistic example would be very useful to have soon, especially if it would allow us to experiment with different ways to average / not average viscosities.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.