HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Normalize Sparse Matrix along rows in C++ Eigen

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
normalizerowseigenalongmatrixsparse

Problem

I wrote a function to normalize sparse matrix along rows using C++ Eigen; however, I feel that it can be improved, and that I am not using some built in functions of Eigen.

#include 
#include 
#include 
#include 
#include 

using namespace Eigen;

std::vector get_weights(SparseMatrix sm);
SparseMatrix row_normalize(SparseMatrix sm);

int main()
{
  MatrixXd silly(6, 3);

  silly  sparse_silly = silly.sparseView();

  SparseMatrix normalized = row_normalize(sparse_silly);
  std::cout  test = normalized * normalized.transpose();
  // Checking that all diagonal elements of non-zero rows are 1
  std::cout  row_normalize(SparseMatrix sm)
 {
   std::vector weights = get_weights(sm);
   SparseMatrix sm_weights(sm.rows(), sm.rows());
   for(int i = 0; i  get_weights(SparseMatrix sm)
{
  std::vector weights;
  for (int i = 0; i < sm.rows(); ++i)
    {
      double my_sum = 0;
      double *val_ptr = sm.row(i).valuePtr();

      if(sm.row(i).nonZeros())
        // avoid division by zero problem
        {
          for (int j = 0; j < sm.row(i).nonZeros(); ++j) 
            {
              my_sum += (*val_ptr * (*val_ptr));
              val_ptr++;
            }
          weights.push_back(1.0 / sqrt(my_sum));
        }
      else
        {
          weights.push_back(0);
        }
    }
  return weights;
}

Solution

-
I'd put the Eigen headers below the STL headers. Also, ` should be , the latter being the C++ library incorporated into the std namespace.

-
You can define
main() below the other functions, allowing you to save a bit of room by eliminating the function prototypes.

-
You don't need that flushing with
std::endl. To get just a newline, output a "\n".

-
main() doesn't need an explicit return 0. As reaching this point implies success, the compiler will do this return for you.

-
You shouldn't use an
int here:

for(int i = 0; i < weights.size(); ++i)
  {
    sm_weights.insert(i,i) = weights[i];
  }


If you had your compiler warnings up high (and they should be), you should've been told about a type-mismatch here. The type for
i should instead be std::size_type (an unsigned type), which is the return type of size() for each STL container class.

-
For this
for loop:

for (int j = 0; j < sm.row(i).nonZeros(); ++j) 
  {
    my_sum += (*val_ptr * (*val_ptr));
    val_ptr++;
  }


val_ptr++ can still be put into the loop statement:

for (int j = 0; j < sm.row(i).nonZeros(); ++j, val_ptr++) 
{
    my_sum += (*val_ptr * (*val_ptr));
}


I'm also unsure about all the raw pointer use here. Raw pointers in C++ should be kept to a minimum, so there may be a way to do this without using any of them.

-
You pass
sm by value in both of your functions, however, it doesn't look like you're trying to allow the compiler to perform move semantics. But you're also using the argument in such a way that it's only being read for writing to other values. In such a case, you should pass sm by const&`.

Code Snippets

for(int i = 0; i < weights.size(); ++i)
  {
    sm_weights.insert(i,i) = weights[i];
  }
for (int j = 0; j < sm.row(i).nonZeros(); ++j) 
  {
    my_sum += (*val_ptr * (*val_ptr));
    val_ptr++;
  }
for (int j = 0; j < sm.row(i).nonZeros(); ++j, val_ptr++) 
{
    my_sum += (*val_ptr * (*val_ptr));
}

Context

StackExchange Code Review Q#61695, answer score: 4

Revisions (0)

No revisions yet.