Did I convert this C++ class to Common Lisp correctly?

Question

Here is the original C++ class I converted to Lisp at the very bottom of this post.

class TrainingData
{
public:
    TrainingData(const string filename);
    bool isEof(void) { return m_trainingDataFile.eof(); }
    void getTopology(vector<unsigned> &topology);

    // Returns the number of input values read from the file:
    unsigned getNextInputs(vector<double> &inputVals);
    unsigned getTargetOutputs(vector<double> &targetOutputVals);

private:
    ifstream m_trainingDataFile;
};

It is a class from a C++ Neural Network I am converting to Common Lisp. The entire 400 line Neural Net is here.

Below is the conversion I came up with. Based on the Neural Network I linked above, can anyone tell me if I converted the C+ class, above, correctly with the following Lisp code? I'd rather have a little assurance before I start converting the entire 400 lines.

(defclass training-data ()
  ((training-data-stream :reader training-data-stream
                         :writer |set training data stream (using a funky name)|
                         :initarg :training-data-stream))
  )


;;; initialize-instance :after -- This is your 'constructor' code.

(defmethod initialize-instance :after ((td training-data)
                                       &key
                                       training-data-stream
                                       &allow-other-keys)
  ;; Initialize the slot.

  ;; Note that 'opening' a stream, as it is implied by the C++ class
  ;; definiton and leaking the open stream handle is very impolite.

  (let ((is (etypecase training-data-stream
              ((string pathname) (open training-data-stream :direction :input))
              (stream (if (input-stream-p training-data-stream)
                          training-data-stream
                          (error "Not an input stream.")))))
        )
    (|set training data stream (using a funky name)| is td)
    ))


(defgeneric is-eof (td)
  (:method ((td training-data))
   ;; Are you sure you want this method?
   ))

EDIT: As far as tests go, please let me explain:

Due to the fact I'm just a intermediate at Linear Algebra I had a wee bit of a time understanding Neural Networks. This conversion into Lisp is the way I figured out how to figure out what Neural Networks do. I'm best at Lisp, pretty good at C, and sort of good at C++, so I'm trying to learn Neural Networks by converting a C++ nn to Lisp.

I don't have enough info to test any specific piece of the code I posted here or at the link yet. I'm still in the process of learning std::stringstream and I've just learned what bitwise XOR was. I can post the instructions that tell how to run the C++ nn at the link in its entirety, and the code to create the data file for it, so you can see how it runs or explain how the incomplete snippet I posted ties in with the rest of the code. The incomplete member functions I added in this post have completed counterparts within the rest of the code at the link I just added.

Answer 1

Code review - mostly style

You did a litteral translation from C++ to CL that seems good. I have some stylistic issues with it:

(defclass training-data ()
  ((training-data-stream :reader training-data-stream
                         :writer |set training data stream (using a funky    name)|
                         :initarg :training-data-stream)))

1. I guess |set training data stream (using a funky name)| is an attempt at obfuscating the writer method, so that you can have a kind of "private" setter. Don't do that.

   1. :writer is optional: if you don't specify it, you won't generate a specific setter for your slot. You can always use slot-value to alter a slot. However, I don't recommend this appropach.

   2. You can define a :accessor method, which acts as both a reader and a writer method (resp. (my-accessor instance) and (setf (my-accessor instance) value)). Since your C++ field is private and has no public getter/setter, this is an appropriate approach:

      • Define only an accessor, say training-data-stream, and
      • Do not export the training-data-stream symbol from your package. Visibility is handled at package level in CL.

2. Your initarg could be made shorter: it is used when making an instance of a training-data class, so :training-data-stream is redundant, just use :input-stream (:stream would be too vague).

Then...

;;; initialize-instance :after -- This is your 'constructor' code.

(defmethod initialize-instance :after ((td training-data)
                                       &key
                                       training-data-stream
                                       &allow-other-keys)
  ;; Initialize the slot.

So far, so good; just rename training-data-stream as input-stream.

  ;; Note that 'opening' a stream, as it is implied by the C++ class
  ;; definiton and leaking the open stream handle is very impolite.

Your own comment highlights that

• the original C++ class might not be good at following C++ best practices (RAII, for example), and
• you are doing a verbatim clone of the C++ class instead of having a more idiomatic CL approach. Since you are just copying an existing codebase in order to understand it, it is not so bad. Maybe you could try to refactor it later, as an exercise.

Besides, "impolite" is not the word I would have used, because it is not accurate (it sounds as-if you ignore, or side-step, the real problems behind leaking resources).

Hereafter, your let binding is useless:

  (let ((is (etypecase training-data-stream
              ((string pathname) (open training-data-stream :direction :input))
              (stream (if (input-stream-p training-data-stream)
                          training-data-stream
                          (error "Not an input stream.")))))
        )
    (|set training data stream (using a funky name)| is td)
    ))

In other word, (let ((x (something))) (use x)) could simply be (use (something)). Moreover, you changed the original behavior by allowing the constructor to be initialized with streams, and not only strings (was it necessary?).

Note that instead of etypecase, which is good, you could also let initialize-instance call a new generic method, say initialize, that dispatches on the type of the stream, either string or stream (we can't dispatch on &key arguments, otherwise initialize-instance would suffice).

    (setf (training-data-stream td)
          (etypecase input-stream
            ((string pathname) (open input-stream :direction :input))
            (stream (if (input-stream-p training-data-stream)
                        training-data-stream
                        (error "Not an input stream."))))))

Your objective

I'm trying to learn Neural Networks by converting a C++ nn to Lisp

Learning high-level concepts like neural networks using an implementation like the one you linked is not very effective, in my opinion.

Even though you have difficulties with C++, this is not the language that prevents you from understanding neural networks; for example:

    Layer &outputLayer = m_layers.back();
    m_error = 0.0;

    for (unsigned n = 0; n < outputLayer.size() - 1; ++n) {
        double delta = targetVals[n] - outputLayer[n].getOutputVal();
        m_error += delta * delta;
    }
    m_error /= outputLayer.size() - 1; // get average error squared
    m_error = sqrt(m_error); // RMS

Those are just math operations, and the fact that they are written in C++ is not what makes them difficult to grasp. In fact, I never really studied neural networks and all the above code is cryptic, even though I am familiar with the language.

I doubt that rewriting it in CL will help you.

What you need first is a book and tutorials for high-level concepts. Experiment with a easy-to-use library, like for example PyBrain, which has a good documentation.

Maybe when you have a good understanding of neural networks, you can go back to the linked code and inspect the implementation to see how it was designed, and why.