Skip to content

WIP: Remove non contiuous option from distance point from curved planes#304

Closed
MFraters wants to merge 23 commits intoGeodynamicWorldBuilder:mainfrom
MFraters:remove_non-contiuous_option_from_distance_point_from_curved_planes
Closed

WIP: Remove non contiuous option from distance point from curved planes#304
MFraters wants to merge 23 commits intoGeodynamicWorldBuilder:mainfrom
MFraters:remove_non-contiuous_option_from_distance_point_from_curved_planes

Conversation

@MFraters
Copy link
Member

@MFraters MFraters commented Jul 4, 2021

This is a first attempt to address issue #302.

This will change the result for the old interpolation schemes and therefore break some tests, but in my opinion the new result is closer to what a use would expect anyway. I see no reason why a user would want to have a slab or fault which is discontiuous at the points which define the location of the slab/fault. If a user wants a discontiuous areas in a slab, they can still create it explicitly.

So if this pull request is implemented, all the slabs will be continuous. At the same time allows this allows for some (hopefully significant) optimizations in the interpolation scheme.

@github-actions
Copy link

github-actions bot commented Jul 4, 2021

Benchmark Master Feature Difference (99.9% CI)
Slab interpolation simple none 1.279 ± 0.032 (s=330) 1.445 ± 0.027 (s=333) +12.3% .. +13.5%
Slab interpolation curved simple none 1.156 ± 0.276 (s=380) 1.264 ± 0.225 (s=367) +4.1% .. +14.6%
Spherical slab interpolation simple none 1.364 ± 0.004 (s=353) 1.386 ± 0.005 (s=304) +1.5% .. +1.7%
Slab interpolation simple curved CMS 2.107 ± 0.017 (s=242) 1.815 ± 0.019 (s=218) -14.1% .. -13.6%
Spherical slab interpolation simple CMS 3.734 ± 0.147 (s=176) 1.365 ± 0.050 (s=184) -64.5% .. -62.4%
Spherical fault interpolation simple none 2.001 ± 0.044 (s=242) 1.971 ± 0.043 (s=213) -2.1% .. -0.8%


if (angle >= 0)
WBAssert(angle >= -const_pi && angle <= const_pi,
"You are tring to use the fast sin function in an invalid range. "
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
"You are tring to use the fast sin function in an invalid range. "
"You are trying to use the fast sin function in an invalid range. "

@MFraters
Copy link
Member Author

MFraters commented Jul 6, 2021

Thanks for taking a look at the code Wolfgang! I fixed the spelling mistake :)

The pull request is currently not ready yet. I am slowly getting closer to the performance I would like (at least no big performance loss and still some values to tune), but to do that I had to cut a lot of corners some which I some certainly will have to reverse. I am happy though that I managed to get the Spherical fault interpolation simple none from about +135% to slightly negative :)

Part of the solution for that particular example is to actually do something similar to #264. This will in no way replace the functionality in that pull request, because it is has expensive code before it and is using an expensive functions to do it. At the current state my test for @alarshi 's model (./bin/WorldBuilderVisualization geodynamic_world_builder_faults.json ../test/visualization/spherical_subducting_plate_gridfile.grid) went from 2min and 4 sec(14403M instructions/cycles according to callgrind) to 1 min and 17 sec. (9595M instructions/cycles).

Some of the speed ups would also have been possible with the old method, although not all. The main questions I am having is whether to still allow the old method for it's speed. That would mean that time needs to be invested in fixing the issue discussed in #302, which will most likely incur a small but significant cost on the performance. I could leave the old method available through a separate function, without the option to interpolate coordinates.

This pull request still requires a bit of work and (a lot of) cleaning up and testing, but I am now happy with the direction it is going.

@bangerth
Copy link
Contributor

bangerth commented Jul 7, 2021

I have trouble identifying what it is you wanted me to look at. Can you point me at a specific commit (or just move unrelated commits into separate PRs)? I'd be happy to look!

@MFraters
Copy link
Member Author

MFraters commented Jul 7, 2021

ah, sorry. I haven't gotten around to clean it up yet. I will certainly ping you when it is ready to take a look. I think a agree with what you said before, and I will focus this pull request on the functional change of removing the old option in favor of the new option. And each optimization will have it's own pull request.

@MFraters MFraters closed this Mar 29, 2022
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