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

Convert water content from percent to a fraction in tian2019 composition model #767

Conversation

danieldouglas92
Copy link
Contributor

I ran into this issue a while ago and I fixed it locally and forgot to open a PR, but I ran into it again just now so I am making sure this is addressed. Within the WorldBuilder the water content output by the tian2019 composition model is in % (i.e. 2%), but ASPECT requires a mass/volume fraction and so for this composition feature to be useful this must be converted to a fraction (i.e 0.02). This PR divides the final value by 100 so that when it is fed to geodynamic modeling codes it is a fraction and not a percent.

@danieldouglas92 danieldouglas92 force-pushed the convert_bound_fluid_to_percentage branch from 47e9fe0 to d4c29b7 Compare November 12, 2024 18:20
Copy link

github-actions bot commented Nov 12, 2024

Benchmark Main Feature Difference (99.9% CI)
Slab interpolation simple none 1.208 ± 0.009 (s=366) 1.206 ± 0.009 (s=382) -0.3% .. +0.0%
Slab interpolation curved simple none 1.219 ± 0.011 (s=368) 1.215 ± 0.007 (s=374) -0.5% .. -0.1%
Spherical slab interpolation simple none 1.223 ± 0.011 (s=378) 1.224 ± 0.009 (s=360) -0.1% .. +0.3%
Slab interpolation simple curved CMS 1.255 ± 0.010 (s=341) 1.253 ± 0.012 (s=379) -0.4% .. +0.1%
Spherical slab interpolation simple CMS 1.637 ± 0.014 (s=258) 1.643 ± 0.014 (s=293) +0.1% .. +0.6%
Spherical fault interpolation simple none 1.229 ± 0.014 (s=368) 1.231 ± 0.009 (s=366) -0.1% .. +0.4%
Cartesian min max surface 2.753 ± 0.018 (s=175) 2.741 ± 0.038 (s=155) -0.8% .. -0.0%
Spherical min max surface 7.716 ± 0.068 (s=60) 7.780 ± 0.292 (s=59) -0.9% .. +2.6%

@danieldouglas92 danieldouglas92 force-pushed the convert_bound_fluid_to_percentage branch from d4c29b7 to ce2420c Compare November 12, 2024 18:46
@danieldouglas92 danieldouglas92 force-pushed the convert_bound_fluid_to_percentage branch from ce2420c to ee2d7dd Compare November 12, 2024 18:53
Copy link
Member

@MFraters MFraters left a comment

Choose a reason for hiding this comment

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

I have no objections against this, but can you add a changelog?

Also, is this not covered by tests, since no tests change?

@danieldouglas92
Copy link
Contributor Author

danieldouglas92 commented Nov 12, 2024

@MFraters Yes I'll definitely add a changelog, and there is a test (gwb-dat/water_content_subducting_plate) that I'll fix, when I made the pull request implementing this feature it gave a non-zero water content, but the test output must have been updated since then and it's now giving a 0 water content everywhere for some reason

@danieldouglas92
Copy link
Contributor Author

The test results were updated in PR #761, but I looked at what they were before this and confirmed that all the water contents are the same as what they were originally (except now being divided by 100).

@MFraters MFraters merged commit 203a7d5 into GeodynamicWorldBuilder:main Nov 14, 2024
40 checks passed
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.

2 participants