Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Why did you call BasicLSTMCell a cell and not a layer? #14693

Closed
nbro opened this Issue Nov 18, 2017 · 4 comments

Comments

Projects
None yet
3 participants
@nbro
Copy link

commented Nov 18, 2017

BasicLSTMCell is actually a layer (as for a layer in MLPs) of LSTM units. Each of these LSTM units contains a cell. Each cell of an LSTM unit contains a scalar value for the CEC and a scalar representing the previous state.

People are usually first introduced to MLPs or feed-forward (and fully connected) neural networks, before being introduced to RNNs and, in particular, LSTMs.

Why would you call BasicLSTMCell a cell, if it can be thought more intuitively (at least for me) as a layer of LSTM units (as I describe them above) containing just one scalar-based cell? Wouldn't it be less ambiguous to call a BasicLSTMCell BasicLSTMLayer???

Moreover, the first parameter to BasicLSTMCell's __init__ method is num_units, i.e. the number of LSTM units, i.e. the number of LSTM cells and gates (if we have 3 gates for every LSTM unit, then the total number of gates in one layer of LSTMs is 3 * num_units).

It almost seems that you created TF to make it as confusing as possible to make it seem hard. It also almost seems that the person who wrote the name of the class BasicLSTMCell is a different person of the person who wrote its __init__ method. What's going on??? A little bit of consistency, for once, no???

A similar argument can be said for MultiRNNCell, which, a lot more intuitively, can be thought as a sequence of layers.

Request

Change classes such as BasicLSTMCell and MultiRNNCell to have more descriptive names of what they actually are in future versions of TF. Then change the corresponding documentation to be more compliant with these changes.

@cy89 cy89 added the type:feature label Nov 18, 2017

@cy89

This comment has been minimized.

Copy link

commented Nov 18, 2017

We work hard on building clean and consistent interfaces, but the truth is that the field moves quickly, and the description and terms used when an idea first breaks out aren't always the same as the best way to teach things turns out a few years later. And we also have a responsibility to our users to maintain some backwards compatibility with the older (and probably uglier) interfaces. I think in practice we can add new, cleaner interfaces, but it's a slower process to deprecate and remove the older, uglier ones.

I think @ebrevdo wrote the code in question (both the class and its init method). He might be able to comment on future, cleaner interfaces.

@ebrevdo

This comment has been minimized.

Copy link
Contributor

commented Nov 19, 2017

Nomenclature varies between different researchers. It's unfortunate that the naming doesn't match standard notation taught by NN courses, but this interface is locked for backwards compatibility until at least tf 2.0 due to our API guarantees. You may have an easier time with the naming in the Keras API. Will add this request to the internal list of possible breaking changes for future versions. Closing.

@ebrevdo ebrevdo closed this Nov 19, 2017

@nbro

This comment has been minimized.

Copy link
Author

commented Nov 19, 2017

@ebrevdo Yes, nomenclature varies, but I am more concerned about consistency within the same class, module and possibly library.

The "cell" BasicLSTMCell contains a method called __init__ whose first parameter is num_units, which is vaguely documented as

The number of units in the LSTM cell.

Number of units??? Which units??? A cell has units???

At least, meanwhile, you should change the documentation to make it more understandable!!

@ebrevdo

This comment has been minimized.

Copy link
Contributor

commented Nov 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.