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

Crash when look up table does not cover the range #6100

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

RanpengLi
Copy link
Contributor

I add an option to make the model crash when the coordinate value is out of the range of the look-up table. I also manually set it for entropy model to make sure the look-up table always cover the entropy-pressure range.

Copy link
Contributor

@bangerth bangerth left a comment

Choose a reason for hiding this comment

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

Strictly speaking, this is an incompatible change. Do you want to document that the entropy reader now needs to be initialized with something that encompasses all possible pressures?

@@ -180,7 +180,8 @@ namespace aspect
*/
double
get_data(const Point<dim> &position,
const unsigned int component) const;
const unsigned int component,
const bool crash_if_not_in_range = false) const;
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 document what the parameter represents in the block above the function?

@@ -855,9 +855,24 @@ namespace aspect
template <int dim>
double
StructuredDataLookup<dim>::get_data(const Point<dim> &position,
const unsigned int component) const
const unsigned int component,
bool crash_if_not_in_range) const
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
bool crash_if_not_in_range) const
const bool crash_if_not_in_range) const


if (crash_if_not_in_range)
{
const std::vector<double> x_coordinates = get_interpolation_point_coordinates(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is expensive because you are copying the whole vector. But you can make it cheaper:

Suggested change
const std::vector<double> x_coordinates = get_interpolation_point_coordinates(0);
const std::vector<double> &x_coordinates = get_interpolation_point_coordinates(0);

Same for the place a few lines down.

@bangerth
Copy link
Contributor

There are also a number of failing tests:

The following tests FAILED:
	312 - entropy_initial_lookup (Failed)
	313 - entropy_initial_lookup_wb (Failed)
	314 - entropy_plasticity (Failed)

What do we want to do about these tests?

@RanpengLi
Copy link
Contributor Author

There are also a number of failing tests:

The following tests FAILED:
	312 - entropy_initial_lookup (Failed)
	313 - entropy_initial_lookup_wb (Failed)
	314 - entropy_plasticity (Failed)

What do we want to do about these tests?

Hi Wolfgang @bangerth , thank you for looking at this pull request! I have modified it and addressed your comments.

For the failed test, 314 failed because the entropy range in the lookup table (600-3000) doesn't cover the model range (up to 3021). I have decreased the maximum entropy in this test setup and will also need to update the result.

For 312 and 313, the model is trying to look up data with pressure = 0, while the look-up table has a pressure range start from 0.25 bar. @lhy11009 Haoyuan, I am mentioning you because you made these two tests. Would it be ok if I just simply change the 0.25 bar to 0 bar in your data table?

For the entropy look-up method, we definitely want the lookup table to cover the full entropy-pressure range of the model. So the three failed tests shows the exact situation we want to avoid with this new assert throw ;)

@RanpengLi RanpengLi force-pushed the crash-if-out-of-range branch from 4470557 to c0d5d62 Compare January 9, 2025 15:48
@RanpengLi RanpengLi changed the title Crash when look up table does not cover the range Crash when look up table does not cover the range [WIP] Jan 9, 2025
@lhy11009
Copy link
Contributor

lhy11009 commented Jan 9, 2025

Hi Ranpeng, thanks for mentioning me in this. Sorry, I didn't see the question in your original post. It's great that you can adjust my tests to accommodate previous failures. I'll do a quick read-through of your commits and see what I can contribute.

@RanpengLi RanpengLi force-pushed the crash-if-out-of-range branch 4 times, most recently from 926fdcf to 65ac4c8 Compare January 10, 2025 15:16
…range, and manually set it for entropy method
@RanpengLi RanpengLi force-pushed the crash-if-out-of-range branch from 5abb63f to b98b59c Compare January 15, 2025 13:47
@RanpengLi
Copy link
Contributor Author

RanpengLi commented Jan 15, 2025

Hi Wolfgang, Haoyuan, and Rene @gassmoeller ,

Thank you for your help on this pull request! I have addressed Wolfgang's comments by:
-Adding a change log.
-Adding a description of the new parameter.
-Including some fixes in the code.

Also, I modified the test setup for the tests that failed with the following changes:

312 - entropy_initial_lookup (Failed) &
313 - entropy_initial_lookup_wb (Failed)

Both crashed because the pressure in the model starts from 0 Pa, but the lookup table starts from 25,000 Pa. To address this, I changed the surface pressure from 0 Pa to 25,000 Pa in the test setup. With the new surface pressure, I tested the results before and after the code modification on my local tester, and the results are identical.

314 - entropy_plasticity (Failed)

This test case crashed because the entropy in the model domain exceeded the range provided by the lookup table. To fix this, I decreased the maximum entropy in the setup. I think this could be more reasonable than including a large lookup table that covers the P-T range of the entire mantle.

@RanpengLi RanpengLi changed the title Crash when look up table does not cover the range [WIP] Crash when look up table does not cover the range Jan 15, 2025
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.

3 participants