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

Use real arguments instead of **kwargs where possible #801

Closed
9 tasks done
huonw opened this issue Feb 6, 2020 · 0 comments
Closed
9 tasks done

Use real arguments instead of **kwargs where possible #801

huonw opened this issue Feb 6, 2020 · 0 comments
Labels
bug Something isn't working doc Issue relates to documentation filler Small task that can be picked up and completed quickly sg-library

Comments

@huonw
Copy link
Member

huonw commented Feb 6, 2020

Describe the bug

Many of the layers have layer-specific required or optional arguments that are passed by manipulating **kwargs, e.g., in GCN:

Args:
units (int): dimensionality of output feature vectors
activation (str or func): nonlinear activation applied to layer's output to obtain output features
use_bias (bool): toggles an optional bias
final_layer (bool): If False the layer returns output for all nodes,
if True it returns the subset specified by the indices passed to it.
kernel_initializer (str or func): The initialiser to use for the weights;
defaults to 'glorot_uniform'.
kernel_regularizer (str or func): The regulariser to use for the weights;
defaults to None.
kernel_constraint (str or func): The constraint to use for the weights;
defaults to None.
bias_initializer (str or func): The initialiser to use for the bias;
defaults to 'zeros'.
bias_regularizer (str or func): The regulariser to use for the bias;
defaults to None.
bias_constraint (str or func): The constraint to use for the bias;
defaults to None.
"""
def __init__(
self, units, activation=None, use_bias=True, final_layer=False, **kwargs
):
if "input_shape" not in kwargs and "input_dim" in kwargs:
kwargs["input_shape"] = (kwargs.get("input_dim"),)
self.units = units
self.activation = activations.get(activation)
self.use_bias = use_bias
self.final_layer = final_layer
self._get_regularisers_from_keywords(kwargs)
super().__init__(**kwargs)
def _get_regularisers_from_keywords(self, kwargs):
self.kernel_initializer = initializers.get(
kwargs.pop("kernel_initializer", "glorot_uniform")
)
self.kernel_regularizer = regularizers.get(
kwargs.pop("kernel_regularizer", None)
)
self.kernel_constraint = constraints.get(kwargs.pop("kernel_constraint", None))
self.bias_initializer = initializers.get(
kwargs.pop("bias_initializer", "zeros")
)
self.bias_regularizer = regularizers.get(kwargs.pop("bias_regularizer", None))
self.bias_constraint = constraints.get(kwargs.pop("bias_constraint", None))

For a specific example, the kernel_initializer argument defaults to "glorot_uniform"; the current set up is unfortunate because:

  • this default has to be specifically and manually documented
  • users' mistakes are flagged less well (e.g. they have to be manually checked for, and IDEs/editors cannot easily autocomplete or validate)
  • our mistakes are flagged less well (e.g. misspelling the argument in the documentation, or renaming it)

If the argument was specified in the __init__ itself similar to bias and dropout, the documentation would list the default automatically, and tooling would be able to reason about it:

image

https://stellargraph.readthedocs.io/en/latest/api.html#stellargraph.layer.gcn.GCN

This is similar to Keras, which has Layer-specific arguments explicitly listed, and arguments for the Layer super-class passed in **kwargs: e.g. https://www.tensorflow.org/api_docs/python/tf/keras/layers/Dense

image

A list of probable suspects

(A search for kwargs[, kwargs.get, kwargs.pop)

$ git grep -n 'kwargs\(\[\|\.get\|\.pop\)'
stellargraph/layer/appnp.py:60:            kwargs["input_shape"] = (kwargs.get("input_dim"),)
stellargraph/layer/attri2vec.py:124:            self.input_node_num = kwargs["node_num"]
stellargraph/layer/attri2vec.py:125:            self.input_feature_size = kwargs["input_dim"]
stellargraph/layer/attri2vec.py:126:            self.multiplicity = kwargs["multiplicity"]
stellargraph/layer/cluster_gcn.py:84:            kwargs["input_shape"] = (kwargs.get("input_dim"),)
stellargraph/layer/cluster_gcn.py:318:            param_value = kwargs.pop(param_name, None)
stellargraph/layer/gcn.py:78:            kwargs["input_shape"] = (kwargs.get("input_dim"),)
stellargraph/layer/gcn.py:89:            kwargs.pop("kernel_initializer", "glorot_uniform")
stellargraph/layer/gcn.py:92:            kwargs.pop("kernel_regularizer", None)
stellargraph/layer/gcn.py:94:        self.kernel_constraint = constraints.get(kwargs.pop("kernel_constraint", None))
stellargraph/layer/gcn.py:96:            kwargs.pop("bias_initializer", "zeros")
stellargraph/layer/gcn.py:98:        self.bias_regularizer = regularizers.get(kwargs.pop("bias_regularizer", None))
stellargraph/layer/gcn.py:99:        self.bias_constraint = constraints.get(kwargs.pop("bias_constraint", None))
stellargraph/layer/gcn.py:347:            param_value = kwargs.pop(param_name, None)
stellargraph/layer/graph_attention.py:140:            kwargs.pop("kernel_initializer", "glorot_uniform")
stellargraph/layer/graph_attention.py:143:            kwargs.pop("kernel_regularizer", None)
stellargraph/layer/graph_attention.py:145:        self.kernel_constraint = constraints.get(kwargs.pop("kernel_constraint", None))
stellargraph/layer/graph_attention.py:148:            kwargs.pop("bias_initializer", "zeros")
stellargraph/layer/graph_attention.py:150:        self.bias_regularizer = regularizers.get(kwargs.pop("bias_regularizer", None))
stellargraph/layer/graph_attention.py:151:        self.bias_constraint = constraints.get(kwargs.pop("bias_constraint", None))
stellargraph/layer/graph_attention.py:154:            kwargs.pop("attn_kernel_initializer", "glorot_uniform")
stellargraph/layer/graph_attention.py:157:            kwargs.pop("attn_kernel_regularizer", None)
stellargraph/layer/graph_attention.py:160:            kwargs.pop("attn_kernel_constraint", None)
stellargraph/layer/graph_attention.py:765:            self.multiplicity = kwargs.get("multiplicity", 1)
stellargraph/layer/graph_attention.py:766:            self.n_nodes = kwargs.get("num_nodes", None)
stellargraph/layer/graph_attention.py:767:            self.n_features = kwargs.get("num_features", None)
stellargraph/layer/graph_attention.py:845:            param_value = kwargs.pop(param_name, None)
stellargraph/layer/graphsage.py:94:            kwargs.pop("kernel_initializer", "glorot_uniform")
stellargraph/layer/graphsage.py:97:            kwargs.pop("kernel_regularizer", None)
stellargraph/layer/graphsage.py:99:        self.kernel_constraint = constraints.get(kwargs.pop("kernel_constraint", None))
stellargraph/layer/graphsage.py:101:            kwargs.pop("bias_initializer", "zeros")
stellargraph/layer/graphsage.py:103:        self.bias_regularizer = regularizers.get(kwargs.pop("bias_regularizer", None))
stellargraph/layer/graphsage.py:104:        self.bias_constraint = constraints.get(kwargs.pop("bias_constraint", None))
stellargraph/layer/graphsage.py:882:            self.n_samples = kwargs["n_samples"]
stellargraph/layer/graphsage.py:883:            self.input_feature_size = kwargs["input_dim"]
stellargraph/layer/graphsage.py:884:            self.multiplicity = kwargs["multiplicity"]
stellargraph/layer/graphsage.py:909:            param_value = kwargs.pop(param_name, None)
stellargraph/layer/graphsage.py:1160:            self.in_samples = kwargs["in_samples"]
stellargraph/layer/graphsage.py:1161:            self.out_samples = kwargs["out_samples"]
stellargraph/layer/graphsage.py:1162:            self.input_feature_size = kwargs["input_dim"]
stellargraph/layer/graphsage.py:1163:            self.multiplicity = kwargs["multiplicity"]
stellargraph/layer/hinsage.py:84:            kwargs.pop("kernel_initializer", "glorot_uniform")
stellargraph/layer/hinsage.py:87:            kwargs.pop("kernel_regularizer", None)
stellargraph/layer/hinsage.py:89:        self.kernel_constraint = constraints.get(kwargs.pop("kernel_constraint", None))
stellargraph/layer/hinsage.py:91:            kwargs.pop("bias_initializer", "zeros")
stellargraph/layer/hinsage.py:93:        self.bias_regularizer = regularizers.get(kwargs.pop("bias_regularizer", None))
stellargraph/layer/hinsage.py:94:        self.bias_constraint = constraints.get(kwargs.pop("bias_constraint", None))
stellargraph/layer/hinsage.py:416:            self.n_samples = kwargs["n_samples"]
stellargraph/layer/hinsage.py:417:            self.input_dims = kwargs["input_dim"]
stellargraph/layer/hinsage.py:418:            self.multiplicity = kwargs["multiplicity"]
stellargraph/layer/hinsage.py:419:            self.subtree_schema = kwargs["input_neighbor_tree"]
stellargraph/layer/hinsage.py:461:            param_value = kwargs.pop(param_name, None)
stellargraph/layer/ppnp.py:61:            kwargs["input_shape"] = (kwargs.get("input_dim"),)
stellargraph/layer/rgcn.py:94:            kwargs["input_shape"] = (kwargs.get("input_dim"),)
stellargraph/layer/rgcn.py:124:            kwargs.pop("kernel_initializer", "glorot_uniform")
stellargraph/layer/rgcn.py:127:            kwargs.pop("kernel_regularizer", None)
stellargraph/layer/rgcn.py:129:        self.kernel_constraint = constraints.get(kwargs.pop("kernel_constraint", None))
stellargraph/layer/rgcn.py:132:            kwargs.pop("basis_initializer", "glorot_uniform")
stellargraph/layer/rgcn.py:134:        self.basis_regularizer = regularizers.get(kwargs.pop("basis_regularizer", None))
stellargraph/layer/rgcn.py:135:        self.basis_constraint = constraints.get(kwargs.pop("basis_constraint", None))
stellargraph/layer/rgcn.py:138:            kwargs.pop("coefficient_initializer", "glorot_uniform")
stellargraph/layer/rgcn.py:141:            kwargs.pop("coefficient_regularizer", None)
stellargraph/layer/rgcn.py:144:            kwargs.pop("coefficient_constraint", None)
stellargraph/layer/rgcn.py:148:            kwargs.pop("bias_initializer", "zeros")
stellargraph/layer/rgcn.py:150:        self.bias_regularizer = regularizers.get(kwargs.pop("bias_regularizer", None))
stellargraph/layer/rgcn.py:151:        self.bias_constraint = constraints.get(kwargs.pop("bias_constraint", None))
stellargraph/layer/rgcn.py:479:            param_value = kwargs.pop(param_name, None)

Here's a checklist (if you choose to work on one, edit this to put your name next to the row, but only check it once the PR merges):

To Reproduce

N/A

Observed behavior

N/A

Expected behavior

Use of **kwargs should be minimised.

Environment

Operating system: Darwin-18.6.0-x86_64-i386-64bit
Python version:

3.7.3 (default, Jul 10 2019, 09:35:06) 
[Clang 10.0.1 (clang-1001.0.46.4)]

Package versions:

absl-py==0.9.0
appdirs==1.4.3
appnope==0.1.0
astor==0.8.1
attrs==19.3.0
backcall==0.1.0
black==19.10b0
bleach==3.1.0
boto==2.49.0
boto3==1.11.10
botocore==1.14.10
cachetools==4.0.0
certifi==2019.11.28
chardet==3.0.4
Click==7.0
coverage==4.5.4
cycler==0.10.0
decorator==4.4.1
defusedxml==0.6.0
docopt==0.6.2
docutils==0.15.2
entrypoints==0.3
gast==0.2.2
gensim==3.8.1
google-auth==1.11.0
google-auth-oauthlib==0.4.1
google-pasta==0.1.8
grpcio==1.27.0
h5py==2.10.0
idna==2.8
importlib-metadata==1.5.0
ipykernel==5.1.4
ipython==7.12.0
ipython-genutils==0.2.0
ipywidgets==7.5.1
isodate==0.6.0
jedi==0.16.0
Jinja2==2.11.1
jmespath==0.9.4
joblib==0.14.1
jsonschema==3.2.0
jupyter==1.0.0
jupyter-client==5.3.4
jupyter-console==6.1.0
jupyter-core==4.6.1
Keras-Applications==1.0.8
Keras-Preprocessing==1.1.0
kiwisolver==1.1.0
llvmlite==0.31.0
Markdown==3.1.1
MarkupSafe==1.1.1
matplotlib==3.1.3
mistune==0.8.4
more-itertools==8.2.0
mplleaflet==0.0.5
nbconvert==5.6.1
nbformat==5.0.4
networkx==2.4
notebook==6.0.3
numba==0.48.0
numpy==1.18.1
oauthlib==3.1.0
opt-einsum==3.1.0
packaging==20.1
pandas==1.0.0
pandocfilters==1.4.2
parso==0.6.1
pathspec==0.7.0
pexpect==4.8.0
pickleshare==0.7.5
pluggy==0.13.1
prometheus-client==0.7.1
prompt-toolkit==3.0.3
protobuf==3.11.3
ptyprocess==0.6.0
py==1.8.1
py-cpuinfo==5.0.0
pyasn1==0.4.8
pyasn1-modules==0.2.8
Pygments==2.5.2
pyparsing==2.4.6
pyrsistent==0.15.7
pytest==5.3.1
pytest-benchmark==3.2.3
pytest-cov==2.8.1
python-dateutil==2.8.1
pytz==2019.3
pyzmq==18.1.1
qtconsole==4.6.0
rdflib==4.2.2
regex==2020.1.8
requests==2.22.0
requests-oauthlib==1.3.0
rsa==4.0
s3transfer==0.3.2
scikit-learn==0.22.1
scipy==1.4.1
seaborn==0.10.0
Send2Trash==1.5.0
six==1.14.0
smart-open==1.9.0
-e git+git@github.com:stellargraph/stellargraph.git@2c513c19c8b8ba931eda3dbf9bf25197821f1ddb#egg=stellargraph&subdirectory=../../../stellargraph-2
tensorboard==2.0.2
tensorflow==2.0.1
tensorflow-estimator==2.0.1
termcolor==1.1.0
terminado==0.8.3
testpath==0.4.4
toml==0.10.0
tornado==6.0.3
traitlets==4.3.3
treon==0.1.3
typed-ast==1.4.1
urllib3==1.25.8
wcwidth==0.1.8
webencodings==0.5.1
Werkzeug==0.16.1
widgetsnbextension==3.5.1
wrapt==1.11.2
zipp==2.1.0

Additional context

N/A

@huonw huonw added bug Something isn't working sg-library doc Issue relates to documentation labels Feb 6, 2020
huonw added a commit that referenced this issue Feb 10, 2020
The headers for the `GCN` and `GraphConvolution` classes now shows all the
information about the class:

```
class stellargraph.layer.gcn.GCN(layer_sizes, generator, bias=True, dropout=0.0, activations=None, kernel_initializer=None, kernel_regularizer=None, kernel_constraint=None, bias_initializer=None, bias_regularizer=None, bias_constraint=None)

class stellargraph.layer.gcn.GraphConvolution(units, activation=None, use_bias=True, final_layer=False, input_dim=None, kernel_initializer='glorot_uniform', kernel_regularizer=None, kernel_constraint=None, bias_initializer='zeros', bias_regularizer=None, bias_constraint=None, **kwargs)
```

Especially for GCN, this will reduce the chance of misspelled arguments being
silently ignored.

See: #801
@huonw huonw added the filler Small task that can be picked up and completed quickly label Feb 12, 2020
huonw added a commit that referenced this issue Mar 19, 2020
If we're going to support this argument, we should include it as a real
argument. This PR switches it to a real argument (and documents it) in the
classes for which it is the only `kwargs` that StellarGraph uses; that is, after
this patch the `kwargs` parameter is not used for anything in these classes
except for passing through to `tf.keras.Layer`.

See: #801
kjun9 added a commit that referenced this issue Mar 24, 2020
kjun9 added a commit that referenced this issue Mar 24, 2020
This fixes GCN to use correct kernel and bias related defaults that match 
the defaults for the `GraphConvolution` class

Related to #801
kjun9 added a commit that referenced this issue Mar 24, 2020
kjun9 added a commit that referenced this issue Mar 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working doc Issue relates to documentation filler Small task that can be picked up and completed quickly sg-library
Projects
None yet
Development

No branches or pull requests

2 participants