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

interface for the distance to a feature's plane #453

Merged

Conversation

lhy11009
Copy link
Contributor

@lhy11009 lhy11009 commented Dec 8, 2022

I added a simple interface that could be used by ASPECT to call the distance_point_from_curved_planes function for computing the distance to a feature's plane surface.
I have also added a test in the unit tests for testing the implementation of the interface.

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.

Thanks for making this pull request! I think this will be a useful addition. To start let's get the tester working. Could you indent the code and add world_builder/objects/distance_from_surface.h?

@lhy11009 lhy11009 force-pushed the distance_to_feature_plane branch from 10e2e40 to dc92a24 Compare December 9, 2022 18:13
@github-actions
Copy link

github-actions bot commented Dec 9, 2022

Benchmark Main Feature Difference (99.9% CI)
Slab interpolation simple none 1.252 ± 0.002 (s=364) 1.251 ± 0.002 (s=357) -0.1% .. -0.0%
Slab interpolation curved simple none 1.192 ± 0.004 (s=372) 1.189 ± 0.005 (s=386) -0.3% .. -0.1%
Spherical slab interpolation simple none 2.007 ± 0.026 (s=215) 2.005 ± 0.030 (s=236) -0.6% .. +0.3%
Slab interpolation simple curved CMS 1.470 ± 0.024 (s=336) 1.467 ± 0.026 (s=279) -0.6% .. +0.3%
Spherical slab interpolation simple CMS 1.773 ± 0.005 (s=260) 1.772 ± 0.010 (s=250) -0.1% .. +0.1%
Spherical fault interpolation simple none 1.669 ± 0.004 (s=275) 1.665 ± 0.004 (s=267) -0.3% .. -0.2%
Cartesian min max surface 3.592 ± 0.035 (s=117) 3.580 ± 0.042 (s=137) -0.8% .. +0.1%
Spherical min max surface 9.656 ± 0.014 (s=46) 9.648 ± 0.020 (s=50) -0.2% .. +0.0%

@lhy11009 lhy11009 force-pushed the distance_to_feature_plane branch from dc92a24 to fb68286 Compare December 10, 2022 05:33
@lhy11009
Copy link
Contributor Author

@MFraters There is some tests failing, I am not sure if I missed something (tests / test_linux_and_macos_mpi ).

@MFraters
Copy link
Member

It took me a bit to get to this issue. The ubuntu-latest tester images where updated and I should concider fixing them to prevent these kind of unexpected problems in the future. Can you rebase?

@lhy11009 lhy11009 force-pushed the distance_to_feature_plane branch from fb68286 to 5d612a5 Compare December 19, 2022 23:54
@lhy11009
Copy link
Contributor Author

@MFraters I have just rebased to the main branch.

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.

Great, the tester is working again. Some small comments, mostly on documentation.

The pull request looks good to me, but can you confirm that it actually does what you want it to do when building your model? Adding a picture of that here would be cool :)

include/world_builder/objects/distance_from_surface.h Outdated Show resolved Hide resolved
include/world_builder/objects/distance_from_surface.h Outdated Show resolved Hide resolved
include/world_builder/objects/distance_from_surface.h Outdated Show resolved Hide resolved
source/world_builder/objects/distance_from_surface.cc Outdated Show resolved Hide resolved
tests/unit_tests/unit_test_world_builder.cc Outdated Show resolved Hide resolved
include/world_builder/world.h Show resolved Hide resolved
@lhy11009 lhy11009 force-pushed the distance_to_feature_plane branch 2 times, most recently from d56fd18 to e28bdd6 Compare December 20, 2022 21:29
@codecov
Copy link

codecov bot commented Dec 20, 2022

Codecov Report

Merging #453 (8fd0518) into main (d46936c) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #453      +/-   ##
==========================================
+ Coverage   90.65%   90.68%   +0.03%     
==========================================
  Files          88       89       +1     
  Lines        6246     6271      +25     
==========================================
+ Hits         5662     5687      +25     
  Misses        584      584              
Impacted Files Coverage Δ
source/world_builder/features/interface.cc 98.07% <100.00%> (+0.07%) ⬆️
source/world_builder/features/subducting_plate.cc 99.65% <100.00%> (+0.01%) ⬆️
...rce/world_builder/objects/distance_from_surface.cc 100.00% <100.00%> (ø)
source/world_builder/world.cc 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d46936c...8fd0518. Read the comment docs.

@lhy11009
Copy link
Contributor Author

@MFraters I have addressed everything in the comment. I am going to implement the designated plugin in aspect with this PR. I am thinking of generalizing that a little bit as well. Let's say the goal is to place particles relative to the surface of a feature defined in the WB. I'll update the progress.

@lhy11009 lhy11009 force-pushed the distance_to_feature_plane branch from e28bdd6 to 8fd0518 Compare December 24, 2022 06:01
@lhy11009
Copy link
Contributor Author

lhy11009 commented Dec 24, 2022

Hi Menno, I have updated the CHANGELOG.md. I also have a better figure than the one I sent to you.

I first put a line of particles in the harzburgite layer by 5 km. The interval between particles is 0.5 km and about 9 thousand particles are entrained in the harzburgite layer (8 thousand are not plotted below the subducting plate).

The interface in this PR is used in the implementation of particles shown in this following figure:

ef_particle_location

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.

That looks great, thanks for contributing it!

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