Avoid the use of intermediate storage arrays in computing bounding boxes.

[bangerth]

Follow-up to #264.

[bangerth]

@alarshi FYI.

[codecov]

Codecov Report

Merging #326 (18e1055) into master (c90d631) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 18e1055 differs from pull request most recent head 4deb49d. Consider uploading reports for the commit 4deb49d to get more accurate results

@@            Coverage Diff             @@
##           master     #326      +/-   ##
==========================================
- Coverage   98.90%   98.90%   -0.01%     
==========================================
  Files          75       75              
  Lines        4277     4275       -2     
==========================================
- Hits         4230     4228       -2     
  Misses         47       47              

Impacted Files	Coverage Δ
source/features/fault.cc	99.16% <100.00%> (-0.01%)	⬇️
source/features/subducting_plate.cc	99.17% <100.00%> (-0.01%)	⬇️

---

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 c90d631...4deb49d. Read the comment docs.

[github-actions]

Benchmark	Master	Feature	Difference (99.9% CI)
Slab interpolation simple none	1.439 ± 0.026 (s=318)	1.445 ± 0.029 (s=309)	-0.1% .. +0.9%
Slab interpolation curved simple none	1.464 ± 0.038 (s=297)	1.464 ± 0.036 (s=320)	-0.6% .. +0.7%
Spherical slab interpolation simple none	1.710 ± 0.036 (s=255)	1.705 ± 0.034 (s=275)	-0.9% .. +0.3%
Slab interpolation simple curved CMS	1.980 ± 0.013 (s=224)	1.979 ± 0.013 (s=233)	-0.2% .. +0.2%
Spherical slab interpolation simple CMS	2.621 ± 0.054 (s=165)	2.611 ± 0.052 (s=182)	-1.1% .. +0.3%
Spherical fault interpolation simple none	1.280 ± 0.008 (s=363)	1.277 ± 0.007 (s=344)	-0.4% .. -0.1%

[alarshi]

@bangerth : Thank you for sharing the modification, I did not know it could be computed this way.

[MFraters]

Thanks for the pull request. Could you elaborate a bit on what the advantage of using lambda functions here is? It is turning four short one line functions into four 5 line function. Is this faster for you computations?

[bangerth]

The idea is this: You already have the coordinates array, and you'd like to find the smallest x value. The current code does this by copying all x coordinates into a separate array and then finding the smallest value via min_element. What my approach does is to just work on the original coordinates array and re-defining what "smallest" means: That's because there is no way to say "this point is smaller than that point" via operator<, but I can define a (lambda) function that compares two points by looking at their x coordinates. So the lambda functions are used to pick out which is the left-/right-/bottom-/top-most point, and at the very end I then only need to query the x or y components of these points.

The code may be slightly more difficult to read, but it avoids the need to copy the data into two arrays.

(It's ok if you don't like the approach and don't want to merge it. It's just something I saw and thought could be made more efficient. But I'm not married to the patch :-) )

[MFraters]

hmm, I can see where you are coming from and that should definitely be more efficient, although I am not seeing much in the benchmarks.

The more I think about it, I do like the approach of not having to copy stuff around needlessly. I am wondering though whether it can be simplified into for example this:

      auto compare_along_x = [](auto p1, auto p2)
      {
        return p1[0]<p2[0];
      };

      auto compare_along_y = [](auto p1, auto p2)
      {
        return p1[1]<p2[1];
      };

      min_along_x = (*std::min_element(coordinates.begin(), coordinates.end(),compare_along_x))[0];
      max_along_x = (*std::max_element(coordinates.begin(), coordinates.end(),compare_along_x))[0];
      min_along_y = (*std::min_element(coordinates.begin(), coordinates.end(),compare_along_y))[1];
      max_along_y = (*std::max_element(coordinates.begin(), coordinates.end(),compare_along_y))[1];

This compiles and passes the tests on my computer, and looks easier to read to me. I think it would be even better if the compare_along_x and compare_along_y functions could be merged, although we now have the slightly weird dimension_index variable which needs to be created before the labmda function and change halfway:

      unsigned int dimension_index = 0;
      auto compare_along_dim = [dimension_index](auto p1, auto p2)
      {
        return p1[dimension_index]<p2[dimension_index];
      };

      min_along_x = (*std::min_element(coordinates.begin(), coordinates.end(),compare_along_dim))[dimension_index];
      max_along_x = (*std::max_element(coordinates.begin(), coordinates.end(),compare_along_dim))[dimension_index];
      
      dimension_index = 1;
      min_along_y = (*std::min_element(coordinates.begin(), coordinates.end(),compare_along_dim))[dimension_index];
      max_along_y = (*std::max_element(coordinates.begin(), coordinates.end(),compare_along_dim))[dimension_index];

This also compiles and passes the tests for me. What is your opinion on these options?

[bangerth]

You're totally right with making this more readable. I've adapted the first idea. I don't think the second one is a benefit because you're changing what the function does based on the dimension_index variable.

[MFraters]

Yes, that looks more readable to me, thanks for making the changes! Could you also apply the same changes to the subducting plate file?

As for the coverage testing, the coverage has not been changed, but for some reason it doesn't recognize min_along_x = (*std::min_element(coordinates.begin(), coordinates.end(), and compare_x_coordinate)) [0]; as being the same statement and only attributing coverage to one of them. Putting them on one line may or may not help, although this is clearly a gcov issue, so this is not blocking a merge. Besides the coverage issue, I personally think it would be more readable if those lines would become one liners instead of being split, since it will be easier to see that the only difference is min element and max element, and I don't think the one liner would be too long. Is there a specific reason you put it on two lines?

[bangerth]

Done as suggested!

[MFraters]

Thanks for the changes and also the small fixes you made along the way :)

This is ready to merge now!