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

Naming network size arguments of GNNModel consistently #30

Closed
Yoshanuikabundi opened this issue Mar 9, 2023 · 1 comment
Closed

Naming network size arguments of GNNModel consistently #30

Yoshanuikabundi opened this issue Mar 9, 2023 · 1 comment

Comments

@Yoshanuikabundi
Copy link
Contributor

Is your feature request related to a problem?

The GNNModel class has 4 arguments that have potentially confusing names and behaviours:

    n_convolution_hidden_features: int
        The number of features in each of the hidden convolutional layers.
    n_convolution_layers: int
        The number of hidden convolutional layers to generate. These are the 
        layers in the convolutional module between the input layer and the 
        pooling layer.
    n_readout_hidden_features: int
        The number of features in each of the hidden readout layers.
    n_readout_layers: int
        The number of hidden readout layers to generate. These are the layers
        between the convolution module's pooling layer and the readout module's
        output layer. The pooling layer may be considered to be both the
        convolution module's output layer and the readout module's input layer.

This can be confusing because...

  1. The usual definition of a hidden layer (as I understand it) are the layers between the input and output layers. In the convolution module, this definition is followed, but in the readout module the input layer is included in the hidden layers. It would also be reasonable to consider the pooling layer a hidden layer in the context of the entire network, but it is not included in either argument.
  2. The word "hidden" is in the features names, but not the layers names. This weakens the link between the features and layers arguments, when semantically they are like two arguments of the same parameter.

Describe the solution you'd like

The new documentation is probably sufficient to remove any ambiguity, but to enhance the semantic link between these arguments, improve clarity for non-readers, and reduce the amount of typing users have to do, I'd suggest removing the word "hidden" from both arguments (but not the documentation)

Describe alternatives you've considered

Lily and I discussed adding the word "hidden" to the arguments where it is missing, but this would not improve clarity and would mean a lot of typing for users. I proposed using the word "generated" instead of "hidden", but this is not a term of art in the field (and is even more typing). We could also add or subtract one from the value to bring the numbers in line with a consistent definition of "hidden", but this would introduce a degree of disconnection between the arguments for minimal benefit (as not all of the hidden layers would have the specified number of features, or else some of the non-hidden layers would have the specified number) and would be confusing.

@lilyminium
Copy link
Collaborator

Closed by #45 -- models now need to have each layer individually specified, which should clear up misunderstandings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants