Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Introduction:

The following two code samples are a direct comparison of performance between Cython and C++. I pieced them together with the help of others-- I barely understand how they work. Their speed in my computer is: 1.225s vs 3.989s (C++ being the faster). The purpose of this comparison is to decide which language I will be using for a series of small but computationally intensive scripts (~200-500 lines).

Question:

As a novice Python programmer dealing with data types, memory (de)allocation, and other low level processes for the very first time, what should I keep in mind when writing Cython modules? Is there anything in the Cython code that can be improved?

Cython script:

import cython
import numpy as np

@cython.boundscheck(False)
@cython.wraparound(False)

cdef class MathObject:
    cdef double a

    def __init__(self, a):
        self.a = a

    # chaotic function (simple math that can't be simplified out by the compiler).
    cpdef double logMap(self):
        self.a = 4.0 * self.a * (1.0 - self.a)
        return 0

    def get_value(self):
        return self.a

def run():
    cdef int i, ii, iii
    cdef int one_thousand = 1000
    cdef double x = 0.2  # logMap has complex behavior at this value

    cdef MathObject objInstance

    objArray = np.empty(one_thousand, dtype=MathObject)
    for i in range(len(objArray)):
        objArray[i] = MathObject(x)

    cdef MathObject[:] objArrayiew = objArray

    for i in range(one_thousand):
        for ii in range(one_thousand):
            for iii in range(one_thousand):
                objInstance = objArrayiew[iii]
                objInstance.logMap()
        print objArray[i].get_value()

    return None

C++ script:

#include <iostream>
using namespace std;

class MathObject {
    public:

    double a;
    MathObject(double b) {
        a=b;
    };

    // simple chaotic function
    double logMap()
    {
        a = 4.0*a*(1.0-a);
    } // logMap
};

int main(int argc, char *argv[])
{
    int ii,jj,kk;

    // create an array of 1000 objects
    int N = 1000;
    MathObject *objArray [N];  // 1000 pointers to objects
    for (ii=0; ii<N; ++ii)  {
        objArray[ii] = new MathObject(0.2);  // construct new objects
    } // for ii

    for (kk=0; kk<1000; ++kk)  {
    for (jj=0; jj<1000; ++jj)  {
    for (ii=0; ii<N; ++ii)  {
        objArray[ii] -> logMap();
    } // for ii
    } // for jj
    } // for kk

    for (ii=0; ii<N; ++ii)  {
        cout << objArray[ii] -> a << "\n";
    } // for ii
}

(Note: I know that this question is a little vague, but most existing tutorials for Cython are written for an audience of C++ programmers. Beginner level information is astoundingly helpful!)

(Second Note: A good explanation of part of the Cython script can be found in the answer to this question.)

share|improve this question

closed as off-topic by jonrsharpe, Emily L., syb0rg, Edward, palacsint Jun 21 at 22:40

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions must involve real code that you own or maintain. Questions seeking an explanation of someone else's code are off-topic. Pseudocode, hypothetical code, or stub code should be replaced by a concrete example." – jonrsharpe, Emily L., syb0rg, Edward, palacsint
If this question can be reworded to fit the rules in the help center, please edit the question.

3 Answers 3

up vote 4 down vote accepted

Translating python to C++ (just a direct line for line translation).

Headers

import cython                                       #include <iostream>
import numpy as np                                  #include <vector>
                                                    #include "Range.h"
@cython.boundscheck(False)
@cython.wraparound(False)

MathObject

cdef class MathObject:                              struct MathObject {
    cdef double a                                       double a;

    def __init__(self, a):                              MathObject(double a)
        self.a = a                                          : a(a)
                                                        {}
    cpdef double logMap(self):                          double logMap() {
        self.a = 4.0 * self.a * (1.0 - self.a)              a = 4.0 * a * (1.0 - a);
        return 0                                            return 0;
                                                        }
    def get_value(self):                                double get_value() {
        return self.a                                       return a;
                                                        }
                                                    };

run

def run():                                          void run() {
    cdef int i, ii, iii
    cdef int one_thousand = 1000                        int one_thousand = 1000;
    cdef double x = 0.2                                 double x = 0.2;

    cdef MathObject objInstance

    objArray = np.empty(one_thousand, dtype=MathObject) std::vector<MathObject> objArray(one_thousand, x);
    for i in range(len(objArray)):
        objArray[i] = MathObject(x)

    cdef MathObject[:] objArrayiew = objArray

    for i in range(one_thousand):                       for(int i: Range(one_thousand)) {
        for ii in range(one_thousand):                      for(int ii: Range(one_thousand)) {
            for iii in range(one_thousand):                     for(int iii: Range(one_thousand)) {
                objInstance = objArrayiew[iii]                      MathObject& objInstance = objArray[iii];
                objInstance.logMap()                                objInstance.logMap();
                                                                }
                                                            }
        print objArray[i].get_value()                       std::cout << objArray[i].get_value() << "\n";
                                                        }

    return None
                                                    }

main

if __name__ == "__main__":                          int main() {
    run()                                               run();
                                                    }

In C++ there is no built in Range type. But it is easy to write:

class Range
{
    int beginValue;
    int endValue;

    struct RangeIterator
    {
        int value;
        RangeIterator(int v) : value(v) {}
        RangeIterator& operator++()     {++value;return *this;}
        int            operator*()      {return value;}
        bool           operator!=(RangeIterator const& rhs) const {return value != rhs.value;}
    };

    public:
        typedef RangeIterator           iterator;
        Range(int e)
            : Range(0, e)
        {}
        Range(int b, int e)
            : beginValue(b), endValue(e)
        {}

        iterator begin() { return RangeIterator(beginValue);}
        iterator end()   { return RangeIterator(endValue);}
};

A couple of notes on the direct translation that I would change:

If you are not going to return a meaningful value (and we notice from usage that the return value is ignored).

double logMap() {
    a = 4.0 * a * (1.0 - a);
    return 0;
}

Then I would make this a void function so you know its not supposed to return anything.

void logMap() {
    a = 4.0 * a * (1.0 - a);
}

I don't like geters they break encapsulation:

double get_value() {
    return a;
}

You usually find that if you are getting values from an object doing stuff with it there is usally a member function (or member of the public API) that can do that. In this case you are only using the value to print. So why not just create a function that knows how to print the value.

friend std::ostream& operator<<(std::ostream& stream, MathObject const& data)
{
    return stream << data.a;
}

If values don't change then mark them as const.

int    const one_thousand = 1000;
double const x            = 0.2;
share|improve this answer
    
A small comment for the archives: It's not necessary to cdef counters in python for loops. Cython recognizes them. (Thank you very much for this guide!!) –  user3465201 Jun 24 at 11:45

Ignoring the Cython part of things for the moment, the C++ code you've posted is open to quite a bit of improvement, with respect to both speed and readability.

using namespace std;

This is generally considered a rather poor habit that it's best to break quickly (or, by strong preference, never form as a habit in the first place).

class MathObject {
    public:

    double a;
    MathObject(double b) {
        a=b;
    };

You usually want to use a member initializer list instead of an assignment inside the body of the constructor when you can (and you can here). That gives code that looks more like this: MathObject(double b) : a(b) {}

// simple chaotic function
double logMap()
{
    a = 4.0*a*(1.0-a);
} // logMap

Two points. First, this is defined as returning a double, but doesn't actually return a value at all. If you try to invoke it like double x = foo.logMap();, you get undefined behavior.

Second, this is the only function in the class other than the constructor. Since you're basically creating an object, and just using it to invoke this one function, it may make sense to consider implementing this as an overload of operator() so you can use this as a function object:

void operator()() {
    a = 4.0*a*(1.0-a);
}

As a final point, the comment noting what a closing brace is closing can be helpful when the opening and closing brace are a long ways apart, but strike me as pretty pointless for a one-line function. About the only time I normally use them is on the closing brace of a namespace (particularly problematic because I don't normally indent all the code inside a namespace).

int main(int argc, char *argv[])

You don't seem to be using argc or argv in this case, so you can just as well leave them out, so the function header would be something like: int main()

int ii,jj,kk;

These don't strike me as the best possible variable names. Given that they're just loop indexes i, j and k are probably enough. If you want longer names, use the extra length to make the names more descriptive. Right now, you have the worst of both worlds: extra length and typing with zero gain in readability. As shown in the code below, a for loop index should normally be defined in the for loop in any case.

// create an array of 1000 objects
int N = 1000;
MathObject *objArray [N];  // 1000 pointers to objects

Two points here. First of all, using the built-in array type in C++ is another of those things that you want to avoid as a rule. You're almost always better off with an alternative, most often std::vector.

Second, avoid using pointers in C++. A lot of teachers and older books can make it seem as if nearly all C++ code needs to use pointers nearly everywhere, but this is actually false. In well-written C++, you'll see very few (if any) pointers at all. Likewise the concomitant use of new--most code shouldn't have to use new (directly) at all.

for (ii=0; ii<N; ++ii)  {
    objArray[ii] = new MathObject(0.2);  // construct new objects
} // for ii

In this case, there's no real reason to use an array of pointers at all. I'd just use a vector of objects: std::vector<MathObject> objArray(N, 0.2);

for (kk=0; kk<1000; ++kk)  {
for (jj=0; jj<1000; ++jj)  {
for (ii=0; ii<N; ++ii)  {
    objArray[ii] -> logMap();
} // for ii
} // for jj
} // for kk

With the change above from array of pointers to vector of objects, as well as adding the overload of operator() to the class, the syntax of the code in the loop changes a little, leaving you with something like this:

for (int i=0; i<1000; i++)
    objArray[i]();

Likewise, where you print out the results:

for (ii=0; ii<N; ++ii)  {
    cout << objArray[ii] -> a << "\n";
} // for ii

This can be changed to use a range-based for loop, much more like Python's, giving something like this:

for (MathObject &&m : objArray)
    std::cout << m.a << "\n";

One more point: in this case, you're not really gaining much by using the class at all. Your entire code seems to work out to something like this:

double a = 0.2;

for (int k=0; k<1000; k++)
    for (int j=0; j<1000; j++)
        a = 4.0*a*(1.0-a);

std::cout << a < "\n";

Although you created 1000 separate objects, you seem to be starting them all from the same initial value and invoking each precisely the same number of times. The function may be chaotic, but the result is still deterministic, so you're basically just computing the same value 1000 times over.

Perhaps you intended to so something different, but at least as I read things, that seems to be equivalent to what you're actually doing. If I had to guess, it would be that what you intended was something more like this:

std::vector<double> a;

a.push_back(0.2);

for (int i=0; i<1000; i++)
    a.push_back(a.back() * 4.0 * (1.0 - a.back());

This will compute and save the values from 1000 consecutive computations. Oh, although I forgot to mention it previously, also note the added white-space inside the computation. IMO, it improves readability somewhat.

share|improve this answer
    
+1 for the "don't need a class here." Even if he had 1000 different values, they can be stored in an array or vector of doubles, without all the OOP overhead, which I don't think adds anything here. –  Jaime Jun 21 at 16:24
1  
@Jaime: What OOP overhead? Are you talking about source or object code. If you are talking source fine. If you are talking object there is none. The size of MathObject is the same as double. –  Loki Astari Jun 21 at 16:53
    
You can't remove the nested loops like that. Unfortunately the call to logMap has side affects and mutates the object. So you need to simulate 1000 * 1000 calls to each object in the array of 1000 elements. –  Loki Astari Jun 21 at 17:22
1  
@LokiAstari: You may well be right--it was a guess and could easily be wrong. Maybe he intended to capture the last 1000 iterations, or maybe he intended to capture every thousandth iteration. As far as "OOP overhead" goes: calling a member function typically means loading the object's this pointer, then accessing the data via that pointer. Although it doesn't use extra data, it can still slow the function call. –  Jerry Coffin Jun 21 at 17:27
    
This was a simple case to test performance. At first, I wasn't sure if Cython would be able to handle this scenario at all! Function calls to a collection of classes is something I will be using a lot. It felt like a good idea for a test. Thank you for the C++ knowledge! –  user3465201 Jun 21 at 17:49

C++ script:

  • The data member a in MathObject should be private. This is necessary because classes are supposed to hide their data members and maintain their state from within the class.

  • Instead of assigning variables in a constructor:

    MathObject(double b) {
        a=b;
    };
    

    use an initializer list:

    MathObject(double a) : a(a) {}
    

    This will also allow you to use the same name for the argument.

  • Consider splitting main() into separate functions, rather than having it do everything. You could have a function for constructing the arrays, looping through them, and displaying them.

  • Do not use a dynamically-allocated array when you can use an std::vector. These will do the memory management for you, which you've failed to do yourself (you use new to allocate memory but never free it with delete).

    std::vector<MathObject> objArray(N, 0.2);
    

    You also don't need to use pointers to objects; just use objects themselves.

  • Loop counter variables should be initialized within the loop statements, not outside. At first, I thought those variables were for something else entirely.

  • The nested for loops should be indented inside of each other, not stacked. It's more readable that way as it clearly shows that they're nested.

  • Remove the "for" comments after each closing curly brace. They're not needed if the loop bodies are small and properly indented if nested.

share|improve this answer

Not the answer you're looking for? Browse other questions tagged or ask your own question.