Over the past couple months, I've written down some notes about ways to clean up the StackNN code.

I think having clean code is important for several reasons. Firstly, it makes it much easier for external users to interact our code, which helps with reproducibility and follow-up experiments. This is timely given that we are about to publish. Additionally, having organized code and documentation lets new members of CLAY get up to speed quickly in the next semester. Another plus of having a clean code base is that we may be able to submit a pull request to integrate StackNN with pytorch.contrib!

With all of these concerns in mind, I have several suggestions for ways that we can clean up the existing code base. We should discuss which of these refactorings are the easiest/most important, as well as whether anything is missing.

Standardizing Class Names with Paper Terminology

• The Controller hierarchy should be renamed to Model. These classes don’t actually affect the nature of the controller being used, but just the overall model architecture. The SimpleStructNetwork hierarchy should be renamed to Controller. These classes specify the architecture of the controller unit which interfaces with the stack.
• Drop “Abstract" from AbstractController (so it will end up just as Model).
• The models.networks subpackage should be made into a top-level package called controllers.

Documentation Style

I propose we adapt documentation of functions and classes to follow the Google Python style guide (what TensorFlow uses) or equivalent. This is somewhat subjective, based on what seems more readable/aesthetically pleasing. Converting current documentation comments to that format shouldn’t be much of an effort, and I think it is much more readable. It can be done on a frequency-of-use basis (Whenever encountering a file, we update its documentation). Example Google-style comment:

def my_function(arg1, arg2):
  """One-line description of function.

  Additional in-depth description goes in paragraphs here.

  Args:
    arg1: This arg does this.
    arg2: This arg does this.

  Returns:
    Always None.
  """
  return None

Performance and Compatibility

• Removing one-hot encodings. In some cases, replacing one-hot encodings with embedding layers might yield decent increases in performance.
• Newest PyTorch. This would be nice so we could integrate new features from a rapidly evolving framework.
• Python 3. Not really a high priority, but something to consider given that Python 2 will be deprecated in 2020.

Random Stuff

• Model and controller become required arguments of task constructor.
• Constructor kwargs to Model (Controller) should not be passed as kwargs to Controller (Network)
• Should we move tasks/configs.py out of tasks package and instead make it top-level?
• Cleaning up constructor arguments/default values. The task class hierarchies have default values for arguments at several layers. This can make it confusing where a value is being set. I propose that all values should only be specified at one layer. For most task parameters, this should probably be in the concrete class. If there is a set of parameters applying to all tasks, we can remove them from the concrete constructors and use an **args parameter in concrete to override them.
• nit: Change Logger to use __enter__/__exit__ hooks instead of __del__. Or use built-in logging module.

Hi all, thank you for publishing your code. I have a question regarding the non-linearity in the output layer of the controller. In their paper, Grefenstette et. al. use sigmoid for the pop and push signals and tanh for the value and output vectors. In your code, you use sigmoids for all outputs. Is that on purpose? Did you experiment with tanh?

Changed implementation of structs to use a list of Variables instead of a big Variable.

Stack and queue test cases pass. On the Linzen task, one epoch takes about 9 minutes with this new implementation.

In the future, we might want to make it so that entries with zero weight in each batch get deleted from the list. I didn't implement this because I wasn't sure it would actually be efficient (Having to check each item of the vector in each batch at every time step).

Fixes #29.

Implemented Linzen agreement task.

The Linzen task now gets 96.7% accuracy after 20 epochs. I disabled our custom initialization in favor of the default initialization, and it increased performance. This could explain the odd LSTM behavior that we have observed more generally.

This is still not replicating the reported results, however. The other main architectural difference as far as I can tell is the softmax versus sigmoid loss function.

The Task class should just have a max_length parameter.

Right now, we have max_x_length and max_y_length at the bottom level, both of which are used in weird and seemingly inconsistent ways.

Most concrete tasks unify these values anyway. Would probably make sense to always make them unified.

Currently, task parameters are passed as a bunch of named arguments. Example from base.py:

class Task(object):
    """
    Abstract class for creating experiments that train and evaluate a
    neural network model with a neural stack or queue.
    """
    __metaclass__ = ABCMeta

    def __init__(self,
                 batch_size=10,
                 clipping_norm=None,
                 criterion=nn.CrossEntropyLoss(),
                 cuda=False,
                 epochs=100,
                 early_stopping_steps=5,
                 hidden_size=10,
                 learning_rate=0.01,
                 load_path=None,
                 l2_weight=.01,
                 max_x_length=10,
                 max_y_length=10,
                 model_type=VanillaModel,
                 controller_type=LinearSimpleStructController,
                 null=u"#",
                 read_size=2,
                 reg_weight=1.,
                 save_path=None,
                 struct_type=Stack,
                 time_function=(lambda t: t),
                 verbose=True):

This should be replaced by a signature of the format:

class Task(object):
    """
    Abstract class for creating experiments that train and evaluate a
    neural network model with a neural stack or queue.
    """
    __metaclass__ = ABCMeta

    def __init__(self, params):

where params is a TaskParams object with configurable parameters. This makes the inheritance patterns between different types of tasks much more readable. It also makes sure that the default values for all the TaskParams are set in exactly one place, which makes debugging easier.

Note: TaskParams could extend dictionary, wrap a dictionary, or params could just be of type dictionary. There are several ways to go about this.

This is to add the code we worked on with Dana for (P)CFG-based language models. The code is not yet complete.

Somehow I can't push to your repository, so I pushed to my fork and made this pull request.

cfg.py contains Dana's version of the code. parentheses.py is my version.

https://github.com/viking-sudo-rm/StackNN/compare/master...yidinghao:master?expand=1#diff-265097babe6c48f38dcedf6a873bd1fc

The control layer takes a basic vector as input and returns a push vector, pop strength, push strength, and read_strength.

The current form of the layer is very basic. The computation is as follows:

• push vector: Linear transformation with tanh.
• push strength: Linear transformation with sigmoid.
• pop strength: Expected value of a softmax distribution produced from a linear transformation.
• read strength: Expected value of a softmax distribution produced from a linear transformation.

We should probably test the layer implemented this way on Reverse, Linzen, etc. I can do some testing when I get back to school in a few days.

Also, there are some extensions worth trying, such as doing multi-pop/multi-read (instead of taking expected value), or allowing multiple read heads in parallel.

Can store stack as a list of Variables instead of a Variable that needs to constantly be reallocated.

This will make pushing, popping, and reading O(1) instead of O(n), which should drastically improve performance.

This PR accomplishes a few things:

• Establishes a mechanism for varying parameters between training and test data for formal tasks (open to discussion about the best way to do this, or if there is already another mechanism for doing this)
• Updates the Dyck task to respect test-specific configurations
• Update the Dyck task configuration to follow the same parameters for testing as what was used in the paper
• Adds support for using the LSTMVisualizer on the Dyck task

It seems to work (and get similar numbers to what was in the paper), but the current implementation slows down training substantially. I still need to dig into why that is (perhaps because max_x_length is now bigger in training?), but should probably wait to merge this until that's solved.

The original Linzen paper used binary cross entropy instead of softmax cross entropy.

https://github.com/TalLinzen/rnn_agreement/blob/master/rnnagr/agreement_acceptor.py

Results are not replicating with softmax cross entropy. We can try switching to binary cross entropy.

I'm not quite sure if the StackNN code is fully CUDA compatible, but recently I had some issues trying to use it.

Should investigate this and add cuda options everywhere in structs/control_layer.