Update the calculation of G tensor so that its form is more consisten…

magmaxt commented 4 months ago

Update the calculation of G tensor so that its form is more consistent with equation 4 of Kaminski&Ribe 2001.

Pull Request Checklist. Please read and check each box with an X. Delete any part not applicable. Ask on the forum if you need help with any step.

Describe what you did in this PR and why you did it.

Like described in the added comments in the code: This update the way to calculate equation 4 of Kaminski&Ribe 2001:

So that the code form now looks more like the equation 4 of Kaminski&Ribe 2001.

Maybe it is more efficient because it replaces two for loops in the past implementation?

Note about texture: rotation_matrix_transposed = inverse of the rotation matrix "the matrix is orthonormal and the inverse of the matrix is equal to its transpose." (see Engler et al., 2024 book: Intro to Texture analysis chp 2.3.2 the rotAtion MAtrix) This operation transform the crystal reference frame to specimen reference frame Accuracy tested, results are the same as the previous implementation.

In the future,

could further simplify the code to compute slip_normal_global and slip_direction_global only once and used them both in bigI and G calcualtions.
should rename parameter slip_cross_product as it may be confused with cross product operatioin because here what is actually doing is outer_product

references: Kaminski, E., and N. M. Ribe. "A kinematic model for recrystallization and texture development in olivine polycrystals." Earth and Planetary Science Letters 189.3-4 (2001): 253-267. Engler, O., Zaefferer, S., & Randle, V. (2024). Introduction to texture analysis: macrotexture, microtexture, and orientation mapping. CRC press.

Before your first pull request:

[x] I have read the guidelines in our CONTRIBUTING.md document.

For all pull requests:

[x] I have followed the instructions for indenting my code.

For new features/models or changes of existing features:

[x] I have tested my new feature locally to ensure it is correct.
[ ] I have created a testcase for the new feature/benchmark in the tests/ directory.
[ ] I have added a changelog entry in the doc/modules/changes directory that will inform other users of my change.

magmaxt commented 4 months ago

Thanks for making the pull request!

Maybe it is more efficient because it replaces two for loops in the past implementation?

The functions you are now calling are internally also looping, so I doubt it is more efficient. Most likely it is slightly less efficient, but I would be surprised if it would be significant. You can test it it if you want, but I think the main advantage of the new way of writing it is the readability.

I see. Thanks for the feedback. I will try to do a test after hackathon.

magmaxt commented 3 months ago

Sorry it took me so long to get to this. I just wanted to run a check whether this is faster or slower and by how much. I ran a similar test to what I do with the world builder using cbdr for 900 seconds and here are the results:
           main           feature        difference (99.9% CI)
sys_time   0.868 ± 0.071  0.916 ± 0.051  [  +1.5% ..   +9.7%]
user_time  7.216 ± 0.133  7.112 ± 0.100  [  -2.4% ..   -0.5%]
wall_time  6.556 ± 0.045  6.498 ± 0.046  [  -1.3% ..   -0.5%]
samples    66             74
If the jenkins rebuild is fine, this is good to merge, otherwise you will have to rebuild.

/rebuild

Thanks so much for looking into this! Thanks for running the speed test. How to understand the speed test results? Would be great if you could teach me next time how to run these speed test so I could also do them on my side and report here. -Xiaochuan

MFraters commented 3 months ago

Sorry it took me so long to get to this. I just wanted to run a check whether this is faster or slower and by how much. I ran a similar test to what I do with the world builder using cbdr for 900 seconds and here are the results:
           main           feature        difference (99.9% CI)
sys_time   0.868 ± 0.071  0.916 ± 0.051  [  +1.5% ..   +9.7%]
user_time  7.216 ± 0.133  7.112 ± 0.100  [  -2.4% ..   -0.5%]
wall_time  6.556 ± 0.045  6.498 ± 0.046  [  -1.3% ..   -0.5%]
samples    66             74
If the jenkins rebuild is fine, this is good to merge, otherwise you will have to rebuild. /rebuild
Thanks so much for looking into this! Thanks for running the speed test. How to understand the speed test results? Would be great if you could teach me next time how to run these speed test so I could also do them on my side and report here. -Xiaochuan

I am thinking or making an manual section about how to do this. For understanding the test results, you can ignore the sys_time (how much time the programs spends on kernel calls I think). The user time is how much the program actually spending on the cpu and the wall time is how much time the program takes if you measure it by have a stop-watch next to your computer. In this case it is nearly the same, but if you run it with many processors, the user/cpu time might be much larger then the wall time.

MFraters commented 3 months ago

If you are interested, it created a graph like this (again, ignore the sys time at the bottom):

results

geodynamics / aspect