Inline most point functions. #301
Conversation
Codecov Report
@@ Coverage Diff @@
## master #301 +/- ##
==========================================
- Coverage 99.00% 98.98% -0.02%
==========================================
Files 74 74
Lines 4227 4155 -72
==========================================
- Hits 4185 4113 -72
Misses 42 42
Continue to review full report at Codecov.
|
|
MFraters
force-pushed
the
inline_point_functions
branch
from
a33f5fc to
296ff04
Compare
MFraters
changed the title
|
It turns out that in inlineing most of (but not all) of the point function has a significant (positive) influence on the performance. I have approached this incrementally both locally and here in this pull request to get a sence of the influence of each set of inlining. The incremental changes can still be seen in the github-actions bot comment by looking at the previous edits and it should match one-to-one with the commits. |
|
Impressive! |
| { | ||
| WBAssert(coordinate_system == point_right.get_coordinate_system(), | ||
| "Cannot substract two points which represent different coordinate systems. Internal has type " << static_cast<int>(coordinate_system) | ||
| << ", other point has type " << static_cast<int>(point_right.get_coordinate_system())); |
There was a problem hiding this comment.
This is true for the dot product operator as well, by the way, even though it doesn't have this assertion.
There was a problem hiding this comment.
Moreover, the formula for the dot product is only correct for Cartesian coordinate systems :-)
| inline | ||
| Point<dim> &operator+=(const Point<dim> &point_right) | ||
| { | ||
| for (unsigned int i = 0; i < dim; ++i) |
| inline | ||
| Point<dim> &operator-=(const Point<dim> &point_left) | ||
| { | ||
| for (unsigned int i = 0; i < dim; ++i) |
There was a problem hiding this comment.
and here and probably some others as well
|
Thanks! I agree with that there should be assert there to prevent mixing of coordinates. Would you be willing to make a quick pull request for this? :P For the dot product in spherical coordinates, I agree that it is not the correct equation, but it is something I am actually using. I am using it in 2d to get an approximation of where along the line between two points you are and I found that it seems accurate enough. I am not sure how much I sinned against mathematics by doing this, but I haven't really though about the correct way of doing this yet. I will make an issue for this. |
I am still testing to see if this make any (positive) difference at all this. So this pull request is currently mostly to see how the difference benchmarks do.