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

fix a bug in the latent heat material & add min and max viscosity #1430

Merged
merged 2 commits into from Apr 10, 2017

Conversation

jdannberg
Copy link
Contributor

This pull request fixes a bug in the latent heat material model: there was an input parameter Viscosity prefactors that was documented and could be set in the input file, but was never used in the code and hence had no actual effect. With these changes, the parameter now does what the documentation says. In addition, parameters for setting a min and max viscosity is introduced. This brings the material to the state we used in our manuscript about numerical methods in ASPECT.

I also want to see if this changes any tests, if not, I will add one.

Copy link
Member

@gassmoeller gassmoeller left a comment

Choose a reason for hiding this comment

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

Thanks for bugfixing! Except for my one question, mostly wording suggestions and typos. Ready to merge if you think the part my question is about is correct, and you address the other comments.

@@ -208,6 +208,7 @@ namespace aspect
// this means, that there are no actual density "jumps", but gradual
// transition between the materials
double phase_dependence = 0.0;
double viscosity_phase_dependence = 1.0;
Copy link
Member

Choose a reason for hiding this comment

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

Please update the comment above for this variable

Copy link
Member

Choose a reason for hiding this comment

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

Also there is a comment in line 181 saying that this part is only computing density, maybe extend that to mention also the phase dependence of the viscosity

@@ -228,6 +229,7 @@ namespace aspect
i);

phase_dependence += phaseFunction * density_jumps[i];
viscosity_phase_dependence *= 1 + phaseFunction * (phase_prefactors[i+1]-1);
Copy link
Member

Choose a reason for hiding this comment

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

why do you use phase_prefactors[i+1] but density_jumps[i]? Maybe there is a reason, but this asymmetry looks strange.

Copy link
Member

Choose a reason for hiding this comment

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

please write the doubles as 1.

@@ -449,6 +453,14 @@ namespace aspect
"viscosity for each phase. "
"List must have one more entry than Phase transition depths. "
"Units: non-dimensional.");
prm.declare_entry ("Minimum viscosity", "1e19",
Patterns::List (Patterns::Double(0)),
Copy link
Member

Choose a reason for hiding this comment

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

Should this really be a list? Or just a double?

"Limit for the minimum viscosity in the model. "
"Units: Pa s.");
prm.declare_entry ("Maximum viscosity", "1e24",
Patterns::List (Patterns::Double(0)),
Copy link
Member

Choose a reason for hiding this comment

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

Same here

}

// as the phase viscosity prefactors are all applied on top of each other, we have
// to scale them here so that they are relative gactors in comparison to the phase above
Copy link
Member

Choose a reason for hiding this comment

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

factors

Copy link
Member

Choose a reason for hiding this comment

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

I would rephrase this a bit (it took me some time to understand what you mean). What about: "as the phase viscosity prefactors are all applied multiplicatively on top of each other, we have to scale them here so that they are relative factors in comparison to the product of all phase above"

@@ -228,6 +229,7 @@ namespace aspect
i);

phase_dependence += phaseFunction * density_jumps[i];
viscosity_phase_dependence *= 1 + phaseFunction * (phase_prefactors[i+1]-1);
Copy link
Member

Choose a reason for hiding this comment

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

please write the doubles as 1.

@@ -242,6 +244,7 @@ namespace aspect
phase_dependence += phaseFunction * density_jumps[i] * (1.0 - composition[0]);
else if (transition_phases[i] == 1) // 2nd compositional field
phase_dependence += phaseFunction * density_jumps[i] * composition[0];
viscosity_phase_dependence *= 1 + phaseFunction * (phase_prefactors[i]-1);
Copy link
Member

Choose a reason for hiding this comment

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

same here

@jdannberg
Copy link
Contributor Author

Okay, I think I addressed all of your comments, and I also added a test.

@@ -0,0 +1,122 @@
# A simple setup for for using the GPlates interface in a 2d shell. See the
Copy link
Member

Choose a reason for hiding this comment

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

can you update the comment for this test please?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, fixed.

Copy link
Member

@tjhei tjhei 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 to me.

@gassmoeller gassmoeller merged commit 76e116d into geodynamics:master Apr 10, 2017
@jdannberg jdannberg deleted the latent_heat_material branch May 8, 2017 15:23
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