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

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 ()
: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.

• The link to the Neural Network code seems to be missing - can you please edit your question to have it please? Jan 28, 2014 at 2:15
• Perhaps you can edit your question to be a bit more focused. It will be hard for someone to understand 400+ lines of C++ code to help you determine if your Lisp implementation is correct. Jan 28, 2014 at 2:17
• And a further comment/question: do you have any tests for this code? If you ported those first then you would be greatly assisted to make sure your implementation is at least ok. Jan 28, 2014 at 2:18

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 ()
: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.


• 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."))))))


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.