Moves to mapper interface

[kylebgorman]

Borrowing a design element I used in UDTube, I decompose the dataset object into two pieces:

• a Mapper interface which knows how to map between lists of strings and tensors (to decode and encode)
• DataSet, as before.

There was no particular reason for the mapper functions to live inside the dataset. This separates the two pieces and uses the mapper object for prediction.

Closes #137. That issue says that the encoding/decoding should be moved to the index, but this actually makes those two even more modular.

(I also imported fix #272 and resolved some merge stuff from #268 etc.)

[Adamits]

I don't really have a problem with it, but I don't actually follow the reasoning for needing another class. The "mapping" operations, to me, could intuitively live in the index, the dataset, or the datamodule. Is the ambiguity there around where it should live actually the problem? Or is this just an OOP principle that someone established?

[kylebgorman]

I don't really have a problem with it, but I don't actually follow the reasoning for needing another class. The "mapping" operations, to me, could intuitively live in the index, the dataset, or the datamodule. Is the ambiguity there around where it should live actually the problem? Or is this just an OOP principle that someone established?

The way to think of the design is this:

• The datamodule creates an index.
• The datamodule creates datasets.
• At the creation of each dataset, a mapper is made from the index and passed to the dataset. (This is necessary because the dataset generates tensors on the fly.)

I am moving it out of the dataset in this PR, because there are a number of places where you want to do what the mapping does but you don't need a reference to the full dataset. (For instance, you don't need the huge list that contains the actual data.) One example of this is in prediction: you need to map but there's no direct reference to the dataset. Putting it in a separate class helps with potential circularity issues.

The other obvious option is to make it part of the index but there are places where we need an index but not a mapper or vice versa. For instance the expert class doesn't need tensors or any of the padding, so it uses the index but not the mapper. I despaired of a way to separate out "string to integer" vs. "integers to tensors" except putting them in separate classes.

The general OOP principle at play here is the Law of Demeter. In the old code (this was part of predict.py) had loader.dataset.decode_target(...); this now reads mapper.decode_target(...).

[Adamits]

Thanks for describing your thinking.

there are a number of places where you want to do what the mapping does but you don't need a reference to the full dataset

Yes good point. I think another place this is helpful is for runtime debugging. previously if I wanted to log encoded inputs/predicted outputs in the forward function at runtime, I needed the dataset.

[Adamits]

LGTM