Allow parameters to be manually set

[JeppeHallgren]

This change allows the user to manually specify the parameters of the CRF. For instance, this means the user can apply masking (say disallowed transitions) to the parameters.

This PR is also helpful for testing (we can now specify the internal state of the CRF).

Tests and linters pass.

[coveralls]

Coverage remained the same at 100.0% when pulling 4dde9fe5ab6056f7ae8abf98c2425b76dbac2b1f on JeppeHallgren:feature/initialize-parameters into 839299625cd2fdae4a9b3d6aa87c230010f7961e on kmkurn:master.

[kmkurn]

Hi, thanks for the PR!

I'm not too sure about changing reset_parameters and allowing users to pass initial weights like that. I like to follow PyTorch's convention to have reset_parameters method accepts no argument and initializes the parameters with default initialization. If users want to set the weights, they already can do so after initialization:

import torch
from torchcrf import CRF

crf = CRF(5)
crf.transitions.data = torch.randn(5, 5)

[JeppeHallgren]

@kmkurn was not aware of the PyTorch convention - I've added a reset_parameters function to the PR that takes no arguments.

Wrt to manually setting the parameters, I'm not sure it is preferred to have the users update the model's internal state directly? There could be a point where we want to perform checks, or update other state, when the user manually updates the weights. I think it is good practice to have a setter function, but let me know what you think.

[kmkurn]

I think since there's no actual private access modifier in Python, the convention is that programmers are assumed to be responsible not to perform something dangerous and if so, they should know what they're doing. So, setter/getter methods are not necessary. As for PyTorch's convention, I believe internal parameters are already accessible even in the built-in modules. For example, if we want to initialize a linear layer using xavier initialization, we have to access the internal weight parameter:

import torch.nn as nn

lin = nn.Linear(2, 3)
nn.init.xavier_uniform(lin.weight)  # we access internal parameter .weight here

So it seems that accessing internal parameters is a very normal thing to do, and I like to keep it that way.

[JeppeHallgren]

I see your point, but not providing a setter forces the programmer to do dangerous things (then there's no supported/upgradeable API for changing the model state) and any update the library will likely break existing code (if we change the internal state at some point). A good example of this is the PyTorch 0.4 upgrade where the Tensor and Variable classes are merged - in this case we could simply update our setter function to check the input type and act accordingly allowing for cross-version support. But without a setter, the dangerous state updates will break.

Also, not sure about your example above. nn.init.xavier_uniform actually calls the setter .uniform() on lin.weight, so it doesn't just change the internal memory directly.

[kmkurn]

I see your point, but I still prefer adhering to the convention. I am not fully convinced that the internal parameters are going to change dramatically. Regarding the PyTorch 0.4 case, a non-backward compatible API change is sort of expected since PyTorch is not stable yet (so is this package). On top of that, even if a setter is provided, there's no guarantee that the users won't do dangerous things because the parameters are still public.

What I meant by that example is how .weight, which is an instance variable of nn.Linear, can be accessed and even modified from outside. If you see nn.Linear code, you'll find this line

self.weight = Parameter(torch.Tensor(out_features, in_features))

and there's no setter methods for that. Anyone can modify the weights anyway he likes. Perhaps a clearer example is if we want to initialize an embedding layer with a pre-trained embedding. nn.Embedding has no setters, so the way to achieve that is:

import torch
import torch.nn as nn

pretrained_emb = torch.randn(10, 5)
emb = nn.Embedding(10, 5)
emb.weight.data = pretrained_emb  # here we set the value for weight directly without any setters

[JeppeHallgren]

@kmkurn I'm still not really happy with the current solution, but I've opened issue #19 instead so other people can join the discussion. Will close this PR for now.