Random uniform distribution deflected as rotation with tests #713

Wang-yijun · 2024-05-29T21:14:42Z

Generated= random uniform distribution with a deflection as rotations and apply these rotations to an input orientation represented with Euler angle or rotation matrix. This grain model is added for all features.
I also removed the old tests for this grain model random_uniform_distribution_deflected using two different random number seed and added new tests.
Some sample generated textures (plotted using MTEX in Matlab):
Input Euler angle (0,0,0), deflection=0.001

Input Euler angle (0,0,0), deflection=1

Input Euler angle (0,90,0), deflection=0.001

Input Euler angle (90,90,0), deflection=0.001

github-actions · 2024-05-29T21:49:45Z

Benchmark	Main	Feature	Difference (99.9% CI)
Slab interpolation simple none	1.015 ± 0.004 (s=446)	0.991 ± 0.005 (s=454)	-2.4% .. -2.3%
Slab interpolation curved simple none	1.020 ± 0.004 (s=433)	0.995 ± 0.004 (s=463)	-2.6% .. -2.4%
Spherical slab interpolation simple none	1.183 ± 0.007 (s=391)	1.161 ± 0.007 (s=379)	-2.0% .. -1.7%
Slab interpolation simple curved CMS	1.061 ± 0.003 (s=447)	1.032 ± 0.004 (s=415)	-2.9% .. -2.7%
Spherical slab interpolation simple CMS	1.594 ± 0.011 (s=299)	1.571 ± 0.012 (s=272)	-1.6% .. -1.2%
Spherical fault interpolation simple none	1.191 ± 0.008 (s=399)	1.167 ± 0.006 (s=366)	-2.1% .. -1.9%
Cartesian min max surface	2.304 ± 0.019 (s=200)	2.317 ± 0.017 (s=192)	+0.3% .. +0.9%
Spherical min max surface	7.227 ± 0.038 (s=69)	7.043 ± 0.035 (s=60)	-2.8% .. -2.3%

Wang-yijun · 2024-05-30T19:58:21Z

More examples on textures with 10,000 grains (plotted with MTEX default colormap in Matlab):
Input Euler angle (0,90,0), deflection=0.001

Input Euler angle (0,90,0), deflection=0.1

Input Euler angle (0,90,0), deflection=0.3

Input Euler angle (0,90,0), deflection=0.6

Input Euler angle (0,90,0), deflection=0.9

Input Euler angle (0,90,0), deflection=1

MFraters

Looks good, thanks for the changes! Great to see that you also added a plume version. I need to look a bit more into some of the code, but here are already some comments.

I think it would make sense to move the matrix_multiply function to a utilities function multiply_3x3_matrices or something similar.
Add a unit test for the matrix multiply function to make sure that it works as expected.

MFraters · 2024-05-30T20:17:08Z

More examples on textures with 10,000 grains (plotted with MTEX default colormap in Matlab): ....

This looks great, thanks!

Wang-yijun · 2024-05-31T00:03:30Z

I've moved the matrix_multiply functions in grain model features into utilities and made a unit test for it.

MFraters

Just a few more comments. Also can you fix the unit tests? You just need to add a few more approx.

Then we can see how the coverage is doing.

MFraters · 2024-05-31T19:28:25Z

include/world_builder/features/plume_models/grains/random_uniform_distribution_deflected.h

+                       const double feature_max_depth) const override final;
+
+          private:
+            // uniform grains submodule parameters


Suggested change

// uniform grains submodule parameters

MFraters · 2024-05-31T19:31:52Z

...ld_builder/features/continental_plate_models/grains/random_uniform_distribution_deflected.cc

+
+                              // Then U' = R * U * R^T
+                              std::array<std::array<double,3>,3> result1 = Utilities::multiply_3x3_matrices(rotation_matrices, basis_rotation_matrices[i]);
+                              // std::array<std::array<double,3>,3> rotated_rotation_matrix = matrix_multiply(result1, rot_T);


Can this line go?

MFraters · 2024-05-31T19:33:12Z

source/world_builder/features/fault_models/grains/random_uniform_distribution_deflected.cc

        }


+
+
+


Five new lines seems a bit excessive to me.

MFraters · 2024-05-31T19:35:50Z

...e/world_builder/features/mantle_layer_models/grains/random_uniform_distribution_deflected.cc

+
+                              // Then U' = R * U * R^T
+                              std::array<std::array<double,3>,3> result1 = multiply_3x3_matrices(rotation_matrices, basis_rotation_matrices[i]);
+                              // std::array<std::array<double,3>,3> rotated_rotation_matrix = multiply_3x3_matrices(result1, rot_T);


MFraters · 2024-05-31T19:37:09Z

.../world_builder/features/oceanic_plate_models/grains/random_uniform_distribution_deflected.cc

+
+                              // Then U' = R * U * R^T
+                              std::array<std::array<double,3>,3> result1 = multiply_3x3_matrices(rotation_matrices, basis_rotation_matrices[i]);
+                              // std::array<std::array<double,3>,3> rotated_rotation_matrix = multiply_3x3_matrices(result1, rot_T);


I fixed this and a few other similar comments and empty lines. I also added approx to all unit test checks.

MFraters · 2024-05-31T23:51:12Z

Some testers fail because std::transform is a c++17 feature, and currently the world builder is a c++14 code. Can you rewrite it in a c++14 way and maybe leave as a comment that once we switch to c++17 we can update it.

Wang-yijun · 2024-06-01T02:39:07Z

I've changed std::transform to for loops.

MFraters

Thanks! It looks good to me now. Please squash the commits as discussed and then it should be ready to merge.

Fix indentation

codecov · 2024-06-04T20:13:01Z

Codecov Report

Attention: Patch coverage is 99.60784% with 1 line in your changes missing coverage. Please review.

Project coverage is 98.27%. Comparing base (dbf0e2c) to head (b44bd4a).
Report is 129 commits behind head on main.

Files with missing lines	Patch %	Lines
...ls/grains/random_uniform_distribution_deflected.cc	98.88%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #713      +/-   ##
==========================================
+ Coverage   98.17%   98.27%   +0.10%     
==========================================
  Files         107      108       +1     
  Lines        7463     7666     +203     
==========================================
+ Hits         7327     7534     +207     
+ Misses        136      132       -4

Files with missing lines	Coverage Δ
...ls/grains/random_uniform_distribution_deflected.cc	`98.92% <100.00%> (+1.70%)`	⬆️
...ls/grains/random_uniform_distribution_deflected.cc	`98.88% <100.00%> (+1.78%)`	⬆️
...ls/grains/random_uniform_distribution_deflected.cc	`98.92% <100.00%> (+1.70%)`	⬆️
...ls/grains/random_uniform_distribution_deflected.cc	`98.92% <100.00%> (+1.70%)`	⬆️
...ls/grains/random_uniform_distribution_deflected.cc	`98.88% <100.00%> (+1.78%)`	⬆️
source/world_builder/utilities.cc	`97.25% <100.00%> (+0.03%)`	⬆️
...ls/grains/random_uniform_distribution_deflected.cc	`98.88% <98.88%> (ø)`

... and 2 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

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

MFraters · 2024-06-04T21:09:56Z

Based on the coverage results, it looks like there is no test for random_uniform_distribution_deflected. Could you add that?

Wang-yijun · 2024-06-04T21:36:05Z

The tests I added are called "random_uniform_distribution_deflected_asrotation_seed1.wb" and "random_uniform_distribution_deflected_asrotation_seed1000.wb". Is this the reason that the tests are not detected?

MFraters · 2024-06-05T20:38:12Z

The tests are run, see https://github.com/GeodynamicWorldBuilder/WorldBuilder/actions/runs/9373443402/job/25807253224?pr=713, but they do not cover the that part of the code. I also just noticed that some comments have not been addressed, can you take another look at them?

Remove unnessary comments and lines

Wang-yijun · 2024-06-05T21:51:38Z

I might have messed up the commits with squash. I've fixed the plume test and addressed the comments above now.

Wang-yijun · 2024-06-06T17:41:08Z

I've added tests to use rotation matrices for initial textures to improve the coverage.

MFraters

Great, thanks for also fixing the coverage! One more thing, can you add a changelog entry in Changelog.md?

Wang-yijun · 2024-06-06T20:09:20Z

Done!

MFraters

Thanks!

…icWorldBuilder#713

Random uniform distribution deflected as rotation with tests

a6616b2

MFraters reviewed May 30, 2024

View reviewed changes

MFraters reviewed May 31, 2024

View reviewed changes

MFraters approved these changes Jun 4, 2024

View reviewed changes

Move matrix multiplication to utilities and add unit test

c576837

Fix indentation

Wang-yijun force-pushed the apply_as_rotation branch from 5659647 to 1aa6e0b Compare June 4, 2024 19:21

Remove std::transform as a c++17 feature

4ad7337

Wang-yijun force-pushed the apply_as_rotation branch from 1aa6e0b to fed9ae7 Compare June 4, 2024 19:33

Fix indentation

8319b98

Fix indentation

Wang-yijun force-pushed the apply_as_rotation branch from fed9ae7 to 8319b98 Compare June 4, 2024 19:50

Fix floating-point comparison with approx

be0e099

Fix plume tests, address comments

b044567

Remove unnessary comments and lines

Wang-yijun force-pushed the apply_as_rotation branch from dc5f50b to b044567 Compare June 5, 2024 21:10

Add tests for using rotation matrices for initial texture

c032d37

MFraters approved these changes Jun 6, 2024

View reviewed changes

Add to CHANGELOG.md

b44bd4a

MFraters approved these changes Jun 6, 2024

View reviewed changes

MFraters merged commit 62f16e7 into GeodynamicWorldBuilder:main Jun 6, 2024
39 of 40 checks passed

MFraters added a commit to MFraters/WorldBuilder-1 that referenced this pull request Jun 8, 2024

Adress comment Timo and change int to size_t in changes from Geodynam…

a6787fe

…icWorldBuilder#713

MFraters added a commit to MFraters/WorldBuilder-1 that referenced this pull request Jun 8, 2024

Adress comment Timo and change int to size_t in changes from Geodynam…

4ec1ea2

…icWorldBuilder#713

MFraters added a commit to MFraters/WorldBuilder-1 that referenced this pull request Jun 10, 2024

Adress comment Timo and change int to size_t in changes from Geodynam…

3bed5ae

…icWorldBuilder#713

MFraters added a commit to MFraters/WorldBuilder-1 that referenced this pull request Jun 10, 2024

Adress comment Timo and change int to size_t in changes from Geodynam…

b703c03

…icWorldBuilder#713

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Random uniform distribution deflected as rotation with tests #713

Random uniform distribution deflected as rotation with tests #713

Wang-yijun commented May 29, 2024

github-actions bot commented May 29, 2024 •

edited

Loading

Wang-yijun commented May 30, 2024

MFraters left a comment

MFraters commented May 30, 2024

Wang-yijun commented May 31, 2024

MFraters left a comment

MFraters May 31, 2024

MFraters May 31, 2024

MFraters May 31, 2024

MFraters May 31, 2024

MFraters May 31, 2024

Wang-yijun May 31, 2024

MFraters commented May 31, 2024

Wang-yijun commented Jun 1, 2024

MFraters left a comment

codecov bot commented Jun 4, 2024 •

edited

Loading

MFraters commented Jun 4, 2024

Wang-yijun commented Jun 4, 2024

MFraters commented Jun 5, 2024

Wang-yijun commented Jun 5, 2024

Wang-yijun commented Jun 6, 2024

MFraters left a comment

Wang-yijun commented Jun 6, 2024

MFraters left a comment

Random uniform distribution deflected as rotation with tests #713

Random uniform distribution deflected as rotation with tests #713

Conversation

Wang-yijun commented May 29, 2024

github-actions bot commented May 29, 2024 • edited Loading

Wang-yijun commented May 30, 2024

MFraters left a comment

Choose a reason for hiding this comment

MFraters commented May 30, 2024

Wang-yijun commented May 31, 2024

MFraters left a comment

Choose a reason for hiding this comment

MFraters May 31, 2024

Choose a reason for hiding this comment

MFraters May 31, 2024

Choose a reason for hiding this comment

MFraters May 31, 2024

Choose a reason for hiding this comment

MFraters May 31, 2024

Choose a reason for hiding this comment

MFraters May 31, 2024

Choose a reason for hiding this comment

Wang-yijun May 31, 2024

Choose a reason for hiding this comment

MFraters commented May 31, 2024

Wang-yijun commented Jun 1, 2024

MFraters left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 4, 2024 • edited Loading

Codecov Report

MFraters commented Jun 4, 2024

Wang-yijun commented Jun 4, 2024

MFraters commented Jun 5, 2024

Wang-yijun commented Jun 5, 2024

Wang-yijun commented Jun 6, 2024

MFraters left a comment

Choose a reason for hiding this comment

Wang-yijun commented Jun 6, 2024

MFraters left a comment

Choose a reason for hiding this comment

github-actions bot commented May 29, 2024 •

edited

Loading

codecov bot commented Jun 4, 2024 •

edited

Loading