Observations and questions on source code

dacamo76

Hello,

I have been digging deep into the source code for classification in RapidMiner while trying to create a minimal library to embed in a Java app.

I have come across a few inconsistencies, in my mind, and a few questions. This will probably be a long post and I am not sure if this is the correct forum, but here goes.

1) In MemoryExampleTable, why does the constructor take a DataRowReader instead of an Iterator<DataRow>. I have an Iterator<DataRow> in my code, and to pass it to a MemoryTable I have to implement DataRowReader or use the strangely named ListDataRowReader, which seems like and extra unnecessary step. Changing this would make the method more flexible by accepting an interface instead of a class and it could be made without affecting existing code.

2) Similarly, the MemoryExampleTable constructor could be changed to accept an Iterable<Attribute> instead of a List<Attribute>, this would have no affect on existing code. I know this is not a big deal, but it can make code that is working with a MemoryTable much cleaner. For example, I am dealing with lazily evaluated Iterables, to create a MemoryTable, I have to convert the Iterable to a List and pass it into the constructor. The method only uses List as an Iterable anyway.

3) Most Java code is probably going to be dealing with Collections or Lists instead of arrays, or at least I believe they should at that level of abstraction. So it is inconvenient to have to convert all the Collections (or Lists or Iterables) into arrays to pass into the the DataRowFactory to create a DataRow. It would seems easier to also have a method that accepts an Iterable and internally call the method that accepts an array. This way, if you only wan to create a DataRow, you don't need to deal with the low-level array conversions. Even better would be for this method to iterate over both inputs and create the DataRow without having to convert the Iterables into an array. The toArray method of Collections makes a copy of the original values, which is unnecessary in this case.

I feel it is inconsistent, to create a MemoryTable with a List of attributes, but create a DataRow, with an array of the exact same attributes.

I know these changes seem trivial, but they really help to make code that uses the library cleaner, and easier to maintain.

dacamo76

Another few questions:

4) The Attribute Interface. I don't understand clone completely, as I've never used it, but from what I gather it should call super.clone(). So I am assuming that the clone() method in the Attribute interface is just a method that returns a copy of the current object. Under this assumption, there is nothing in the class docs that says if it is a deep copy or a shallow copy, so digging deeper I see that it is a mixture of both.
The NominalAttributes don't copy the mappings, they keep the same references. (There is actually a call to clone the mappings in the copy constructors but it is commented out.)
None of this behavior is documented, so you can't be sure what is going on by just looking at the javadocs. At worst, you will get unexpected behavior by assuming it is a complete copy. At best, you dig through the source code and see what is really going on.

This is also the case with the setMapping(NominalMapping) method for the nominal attributes.
In this case it is worse, as both override the same method but cause different side-effects.
The BinominalAttribute method just assigns the new mapping, so the calling class keeps a reference to the internal mapping.
The Polynominal method creates a copy of the new mapping and assigns the copy. The calling class has no reference to the internal mapping.
This behavior was causing a few problems in my code as I did not understand what was going on for some time.
I am not sure which of the two is the correct behavior for the method. The javadocs for the method is just a copy of the getMapping method.

The reason I encountered these errors was while I was trying to create attributes to create an ExampleTable to pass to a model.
I could not use any of the clone() or copy constructor methods and get them to work. They always kept the original mappings and I got an error while creating a new ExampleTable. So I ended up having to create a whole new set of Attributes using the AttributeFactory method passing in the name and type of the attribute.

From the equals() method I gather that an Attribute is considered equal if it has the same name and table index. It does not take into account the type of attribute or the mappings if it is a nominal attribute. Is this correct?

If I can get my head around all this and figure out what the actual intentions of these methods are, I would be more than willing to write the corresponding javadoc to document a strict contract for the methods, this way saving other people the same pain I have gone through.

I know this is a long rant, but I am trying to figure out what is the expected behavior of these methods.

Cheers,

Daniel