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

Adjust default vs vp values #2189

Merged
merged 4 commits into from Apr 30, 2018

Conversation

gassmoeller
Copy link
Member

Closes #1562. Lets see how many tests break.

Copy link
Contributor

@naliboff naliboff left a comment

Choose a reason for hiding this comment

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

One question regarding documentation. Looks like it only broke 10% of the tests ;)

a signaling Nan. This ensures every plugin that creates seismic outputs needs
to use a material model that actually fills those outputs.
<br>
(Rene Gassmoeller, 2018/04/27)
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be worth adding this documentation to the seismic_anomalies post-processor description? Alternatively, is there already an assert that produces a similar error message when the values are filled with Nan?

@gassmoeller gassmoeller force-pushed the adjust_default_vs_vp_values branch 2 times, most recently from da3a0f0 to 3e35b91 Compare April 27, 2018 23:48
Copy link
Contributor

@jdannberg jdannberg left a comment

Choose a reason for hiding this comment

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

Okay, makes sense. Let's see what the tester says.

(out.template get_additional_output<MaterialModel::SeismicAdditionalOutputs<dim> >() != 0);

AssertThrow(material_model_provides_seismic_output,
ExcMessage("You requested the 'Seismic' postprocessor, "
Copy link
Contributor

Choose a reason for hiding this comment

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

please correct the name of the postprocessor

(out.template get_additional_output<MaterialModel::SeismicAdditionalOutputs<dim> >() != 0);

AssertThrow(material_model_provides_seismic_output,
ExcMessage("You requested the 'Seismic' postprocessor, "
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

variables.push_back("Vp");
if (std::find( output_variables.begin(), output_variables.end(), "Vs") != output_variables.end())
{
AssertThrow(material_model_provides_seismic_output,
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move this assert above line 377 and combine it with the one below?

@gassmoeller
Copy link
Member Author

I addressed the comments and updated the test results. The one issue here is that the column numbers in the depth_average output files will change if no seismic velocities are supplied by the material model. This can break postprocessing scripts (only for columns viscosity and vertical heat flux). I do not see a way to prevent this though.

Copy link
Contributor

@jdannberg jdannberg left a comment

Choose a reason for hiding this comment

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

You should also go through all cookbooks and see if they include anything that would need an update in the column number.
Otherwise, this looks good. From my side, this is ready to be merged once you've addressed these small changes.

@@ -0,0 +1,5 @@
Changed: The default value for AdditionalSeismicOutputs was changed from -1 to
a signaling Nan. This ensures every plugin that creates seismic outputs needs
to use a material model that actually fills those outputs.
Copy link
Contributor

Choose a reason for hiding this comment

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

If there's no way around changing the change in column number in the depth average file, that should be documented here too.

@gassmoeller
Copy link
Member Author

I ran the check.sh script in benchmarks and cookbooks, and checked that the seismic velocity columns are not referenced in the manual or somewhere else. Good to go?

@jdannberg jdannberg merged commit 9eeb902 into geodynamics:master Apr 30, 2018
@gassmoeller gassmoeller deleted the adjust_default_vs_vp_values branch April 30, 2018 21:38
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