Draft: Add missing functions in primal #1313
Conversation
src/axom/primal/geometry/Point.hpp
Outdated
| @@ -113,6 +114,7 @@ class Point | |||
| * \return d the dimension of the point. | |||
| * \post d >= 1. | |||
| */ | |||
| AXOM_HOST_DEVICE | |||
| static int dimension() { return NDIMS; }; | |||
There was a problem hiding this comment.
Should this be a constexpr function?
There was a problem hiding this comment.
Thanks for adding these @adayton1 -- looks good to me, but I'd have a bit more confidence if there were a few unit tests, especially for the new Vector::cross implementation in 2D and 3D.
I'd also like to hear more about the need for both Vector::normalize() and Vector::unitize()
| /*! | ||
| * \brief In-place normalization of the vector. | ||
| */ | ||
| AXOM_HOST_DEVICE | ||
| void normalize(); | ||
|
|
||
| /*! | ||
| * \brief In-place unitization (normalization) of the vector. | ||
| */ | ||
| AXOM_HOST_DEVICE | ||
| void unitize(); |
There was a problem hiding this comment.
Suggestion: Is there a good reason to have two functions that do the same thing?
If not, my preference would be to only have normalize() (which is the conventional way to refer to this operation).
| template <typename T, int NDIMS> | ||
| AXOM_HOST_DEVICE inline void Vector<T, NDIMS>::normalize() | ||
| { | ||
| const double len_sq = squared_norm(); | ||
|
|
||
| if(len_sq >= primal::PRIMAL_TINY) | ||
| { | ||
| m_components /= (std::sqrt(len_sq)); | ||
| } | ||
| else | ||
| { | ||
| // TODO: Would it be better to do nothing if the vector is really the | ||
| // zero vector? Or if it is tiny but not the zero vector, find | ||
| // the component with the largest magnitude and set it to 1 (all | ||
| // the other components would then be set to 0). | ||
|
|
||
| // Set the first component to 1 and all others to 0 | ||
| m_components[0] = static_cast<T>(1.); | ||
|
|
||
| for(int i = 1; i < NDIMS; ++i) | ||
| { | ||
| m_components[i] = static_cast<T>(0.); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Should Vector::unitVector() be implemented in terms of this one?
Here's the current implementation:
axom/src/axom/primal/geometry/Vector.hpp
Lines 489 to 507 in 379db95
| //------------------------------------------------------------------------------ | ||
| template <typename T, int NDIMS> | ||
| AXOM_HOST_DEVICE inline T Vector<T, NDIMS>::cross(const Vector<T, NDIMS>& vec) const | ||
| { | ||
| return cross_product(*this, vec); | ||
| } |
There was a problem hiding this comment.
We might need to be careful about this one -- the current cross_product overloads are defined as:
cross_product(Vector3, Vector3) -> Vector3cross_product(Vector2, Vector2) -> Vector3// where the x- and y- components are 0
axom/src/axom/primal/geometry/Vector.hpp
Lines 567 to 586 in 379db95
| AXOM_HOST_DEVICE | ||
| inline Vector<T, NDIMS> Vector<T, NDIMS>::make_vector(const T& x, | ||
| const T& y, | ||
| const T& z) |
There was a problem hiding this comment.
Side note (outside the scope of this PR):
The make_vector utility function was introduced before we had an initializer list constructor for a primal::Vector. I'm not sure if we still need the make_vector helper function. (and similarly for Point::make_point)
Summary
Resolves #1309