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
Multicomponent entropy method - average the looked-up material properties #5622
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, this moves in the right direction. I think the functionality you added is fine, but I have some suggestions for the code structure. Take a look if my suggestions work for you.
|
||
/** | ||
* Seismic P wave velocity for each composition and phase. | ||
*/ | ||
std::vector<double> vp; | ||
|
||
/** | ||
* Seismic S wave velocity for each composition and phase. | ||
*/ | ||
std::vector<double> vs; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still dont think adding these variables into EquationOfStateOutputs
is a good idea, and I also dont think it is necessary as I will outline below. I agree that you can argue that vp
and vs
could be quantities computed by an equation of state, but most of our equations of state do not compute the quantities (and it is not necessary for the inner workings of the model anyway, they are just output quantities). Introducing them here adds them for all of our material models, and that can lead to a number of problems or at best worse performance (although all tests seem to pass for now).
/** | ||
* The thermodynamic lookup equation of state. | ||
*/ | ||
EquationOfState::ThermodynamicTableLookup<dim> equation_of_state; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So here you introduce a new member object into the EntropyModel
class. But I dont see where you actually use this member. Is it necessary? The ThermodynamicTableLookup
duplicates a lot of the functionality of the EntropyModel
class, and eventually we may want to replace some parts of EntropyModel
by using ThermodynamicTableLookup
instead, but that doesnt seem like it happened in this PR.
@@ -163,6 +164,9 @@ namespace aspect | |||
const unsigned int projected_density_index = this->introspection().compositional_index_for_name("density_field"); | |||
const unsigned int entropy_index = this->introspection().compositional_index_for_name("entropy"); | |||
|
|||
std::vector<EquationOfStateOutputs<dim>> eos_outputs (in.n_evaluation_points(), material_file_names.size()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you do not need a std::vector
of EquationOfStateOutputs
. If you check the for-loop below, you only ever access eos_outputs[i]
at a time, you never need all of eos_outputs
. So you may as well make this simply a EquationOfStateOutputs<dim> eos_outputs(material_file_names.size())
and then access that single object in the loop below. This is different for the Steinberger
material model (it accesses the whole vector of eos_outputs in line 278).
The same is true for the volume_fractions object in the next line.
for (unsigned int j=0; j<material_file_names.size(); ++j) | ||
{ | ||
eos_outputs[i].vp[j] = entropy_reader[j]->seismic_vp(entropy, pressure); | ||
eos_outputs[i].vs[j] = entropy_reader[j]->seismic_vs(entropy,pressure); | ||
} | ||
|
||
seismic_out->vp[i] = MaterialUtilities::average_value (volume_fractions[i], eos_outputs[i].vp, MaterialUtilities::arithmetic); | ||
seismic_out->vs[i] = MaterialUtilities::average_value (volume_fractions[i], eos_outputs[i].vs, MaterialUtilities::arithmetic); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this is the place why you introduced vp
and vs
into EquationOfStateOutputs
. But you should be able to write this just as well as:
for (unsigned int j=0; j<material_file_names.size(); ++j) | |
{ | |
eos_outputs[i].vp[j] = entropy_reader[j]->seismic_vp(entropy, pressure); | |
eos_outputs[i].vs[j] = entropy_reader[j]->seismic_vs(entropy,pressure); | |
} | |
seismic_out->vp[i] = MaterialUtilities::average_value (volume_fractions[i], eos_outputs[i].vp, MaterialUtilities::arithmetic); | |
seismic_out->vs[i] = MaterialUtilities::average_value (volume_fractions[i], eos_outputs[i].vs, MaterialUtilities::arithmetic); | |
std::vector<double> vp (material_file_names.size()); | |
std::vector<double> vs (material_file_names.size()); | |
for (unsigned int j=0; j<material_file_names.size(); ++j) | |
{ | |
vp[j] = entropy_reader[j]->seismic_vp(entropy,pressure); | |
vs[j] = entropy_reader[j]->seismic_vs(entropy,pressure); | |
} | |
seismic_out->vp[i] = MaterialUtilities::average_value (volume_fractions[i], vp, MaterialUtilities::arithmetic); | |
seismic_out->vs[i] = MaterialUtilities::average_value (volume_fractions[i], vs, MaterialUtilities::arithmetic); |
Can you give this a try?
vp(n_individual_compositions_and_phases, numbers::signaling_nan<double>()), | ||
vs(n_individual_compositions_and_phases, numbers::signaling_nan<double>()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the part why I am so worried about introducing additional variables into EquationOfStateOutputs
. You fill these variables here with signaling_nan, which will cause floating point exceptions for everyone who tries to access the variables. But users of this class (mostly material model plugins) dont know which equations of state correctly fill these variables (or they just assume every equation of state fills every variable). This can lead to unexpected crashes. Also some equations of state already handle seismic velocities (like ThermodynamicTableLookup
, look for the function fill_seismic_velocities
in that class), but they do it without using this new variable. So that is even more confusing from the outside, why would there be a variable vp
that is inside EquationOfStateOutputs
, but the equation of state ThermodynamicTableLookup
doesnt use that and instead offers a different way to compute seismic velocities.
eos_outputs[i].densities, | ||
true); | ||
|
||
out.densities[i] = MaterialUtilities::average_value (volume_fractions[i], eos_outputs[i].densities, MaterialUtilities::arithmetic); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is exactly what you want and should work just fine. Can you already add a test case that shows the functionality is working, or are there any pieces missing? I would guess that in the current state the entropy to temperature conversion would only happen correctly for the first composition table, but all other material properties are already averaged correctly?
Following #5615 which allows the entropy model to read in multiple files, I make the entropy model loop over look-up tables for all components, then average the material properties according to the components' volume or mass fractions.