Prefer C++ headers
If you #include <cmath>
instead of <math.h>
, the mathematical functions will be safely and unambiguously in the std
namespace.
Pass read-only parameters by const reference
Instead of copying the input vectors, it's usually more efficient to pass const references to them:
double cosine(const std::vector<double>& A, const std::vector<double>& B, bool similarity);
double euclidean(const std::vector<double>& A, const std::vector<double>& B, bool similarity);
double distance(const std::vector<double>& A, const std::vector<double>& B,
Metric metric, bool similarity = false);
Use auto
to deduce types
Instead of writing
std::vector<double>::iterator A_iter = A.begin();
std::vector<double>::iterator B_iter = B.begin();
We can ask the compiler to deduce the correct type:
auto A_iter = A.begin();
auto B_iter = B.begin();
This also makes it easier if you ever want to change the type of A
and B
(perhaps even to a template argument type). In fact, by changing to const references, we already changed the type (to std::vector<double>::const_iterator
), but auto
isolates us from that.
Use the right kind of exception
You could be more specific, and use std::invalid_argument
for the error type here:
if (d_a == 0.0 || d_b == 0.0) {
throw std::invalid_argument("Vectors must have a magnitude greater than zero");
}
I've changed the wording above, because length
usually means the number of elements when we're using standard collections.
I also changed the zero constant to be a double
, the same as d_a
and d_b
, since it will get promoted for the comparison anyway. Only use float
when you really need the reduced precision.
Don't compare booleans against false
This is hard to read:
if (similarity == false) {
// negative case
} else {
// positive case
}
You can re-write it as
if (similarity) {
// positive case
} else {
// negative case
}
This will normally produce identical code, but the cognitive overhead is reduced.
Avoid non-standard extensions
Although many implementations define a constant called M_PI
, this isn't specified by any standard. Thankfully it's easy to make your own value, using the knowledge that tan(¼π) is 1:
static const auto pi = 4*std::atan(1);
Avoid std::pow(x, 2)
To square a number, it's simpler and more efficient to multiply it by itself. pow()
is very general (handling non-integer powers), and usually works by multiplication of logarithms.
Even though std::pow()
has overloads for integer powers, I find that for squaring, it's easier to read a simple multiplication rather than a function call. It's worth introducing a temporary to avoid that:
//sq_dist += pow(*A_iter - *B_iter, 2);
auto dist = *A_iter - *B_iter;
sq_dist += dist * dist;
Don't fall off the end of non-void functions
Here, we're assuming that we're always passed a valid enum constant:
switch (metric) {
case Metric::Cosine:
return cosine(A, B, similarity);
case Metric::Euclidean:
return euclidean(A, B, similarity);
}
// implicit return
Robust code checks that we don't flow out of the switch
:
switch (metric) {
case Metric::Cosine:
return cosine(A, B, similarity);
case Metric::Euclidean:
return euclidean(A, B, similarity);
}
throw std::invalid_argument("Invalid metric");
Invalid enum values shouldn't happen, but it costs little to be defensive against a mistaken cast.
Design issues
It's not clear whether cosine()
and euclidean()
are intended to be part of the public interface. If they are not, then give them internal linkage (e.g. by use of the anonymous namespace, or with the static
keyword), and consider moving the similarity
parameter out of them.
I prefer to avoid booleans that change the meaning of functions like that; see if you agree when you see my full example.
Here's a full version with the above issues all addressed (and assuming that cosine()
and euclidean()
are intended to be internal detail).
Declarations
This is what I'd put in the header file (let's call it similarity.h
):
#ifndef SIMILARITY_H
#define SIMILARITY_H
#include <vector>
enum class Metric { Cosine, Euclidean };
double distance(const std::vector<double>&, const std::vector<double>&, Metric);
double similarity(const std::vector<double>&, const std::vector<double>&, Metric);
#endif
Implementation
Having included the header, here's the C++ file:
#include "similarity.h"
#include <cmath>
#include <stdexcept>
namespace {
double cosine_distance(const std::vector<double>& a, const std::vector<double>& b)
{
double mul = 0.0;
double d_a = 0.0;
double d_b = 0.0;
auto ia = a.begin();
auto ib = b.begin();
for (; ia != a.end(); ia++, ib++) {
mul += *ia * *ib;
d_a += *ia * *ia;
d_b += *ib * *ib;
}
if (d_a == 0.0 || d_b == 0.0) {
throw std::invalid_argument("Vectors must have a magnitude greater than zero");
}
return mul / std::sqrt(d_a * d_b);
}
double euclidean_distance(const std::vector<double>& a, const std::vector<double>& b)
{
double sq_dist = 0.0;
auto ia = a.begin();
auto ib = b.begin();
for (; ia != a.end(); ia++, ib++) {
auto dist = *ia - *ib;
sq_dist += dist * dist;
}
return std::sqrt(sq_dist);
}
}
double distance(const std::vector<double>& a, const std::vector<double>& b, Metric metric)
{
if (a.size() != b.size())
throw std::invalid_argument("a and b must be the same size!");
if (a.size() < 1)
throw std::invalid_argument("Empty vectors belong to no metric space");
switch (metric) {
case Metric::Cosine:
return cosine_distance(a, b);
case Metric::Euclidean:
return euclidean_distance(a, b);
}
throw std::invalid_argument("Invalid metric");
}
double similarity(const std::vector<double>& a, const std::vector<double>& b, Metric metric)
{
static const auto pi = 4*std::atan(1);
switch (metric) {
case Metric::Cosine:
return std::acos(distance(a, b, metric)) / pi;
case Metric::Euclidean:
return 1.0 / distance(a, b, metric);
}
throw std::invalid_argument("Invalid metric");
}
Note that I include our own header first. That's a helpful approach, to improve our chances of discovering if it accidentally depends on something it doesn't explicitly include.
main()
, there's very little to go on. See How to Ask for some advice to give your question a better title. \$\endgroup\$main
here would orient feedback toward an aspect of this code that I'm not particularly curious about. There's an interesting meta question here, but as it stand I really believe I've asked an appropriate question. \$\endgroup\$main()
would be nice (for example, if it included test cases), but it's not required. \$\endgroup\$