-
Notifications
You must be signed in to change notification settings - Fork 225
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
Kernel function extension #558
base: master
Are you sure you want to change the base?
Conversation
class Periodic(BaseKernel): | ||
def __init__( | ||
self, | ||
tau: torch.Tensor = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would advocate for just having period
as a more interpretable argument name here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given we call 'bandwidth' as 'sigma', I was wondering if it is better to keep all parameters as greek letters for consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rather be in favour of renaming sigma
to bandwidth
(is this PR backwards incompatible without renaming?) and tau
to period
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those suggestions sounds good to me.
self.log_tau = nn.Parameter(torch.empty(1), requires_grad=trainable) | ||
self.init_required = True | ||
else: | ||
self.log_tau = nn.Parameter(tau.log(), requires_grad=trainable) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are going to the effort to make parameters trainable we should probably make it possible to train one and not the other, particularly given the period will (almost always?) be fixed according to domain specific knowledge whereas one may wish to train the bandwidth.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new parameter implementation should allow such behaviour. But at the moment for existing kernels, these are still specified by the global trainable
argument. Guess the better solution here is to add a trainable
option for each parameter.
Nice work! Indeed the previous coupling to A few things that may be worth further consideration:
|
Self-requesting a review for this since I expect this PR will require moderate changes to the save/load functionality. |
…parameter implementation for the general kernel class. (3) added an initial example notebook.
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
…1) modify the type of composite kernels as BaseKernel, and change the type signatures accordingly. (2) remove the feature dimension option in BaseKernel. (3) add specific tests on parameter inference. (4) remove numpy inputs from kernels with pytorch backend. (5) misc minor fixes following previous comments.
…parameter implementation for the general kernel class. (3) added an initial example notebook.
…ow allows: (1) any sum and product with the direct add and multiply equation. (2) the dimension selection is built-in with the main class. (3) the deep kernel is also implemented with the new base class and the user can access it as a single composite kernel.
…r messages on unsupported operations. Also added new notebook on creating user-defined kernels for drift detectors.
…pe hint for various methods and attributes for better consistency.
…divide input types.
…(3) type hints and (4) misc fixes.
…1) modify the type of composite kernels as BaseKernel, and change the type signatures accordingly. (2) remove the feature dimension option in BaseKernel. (3) add specific tests on parameter inference. (4) remove numpy inputs from kernels with pytorch backend. (5) misc minor fixes following previous comments.
…or nested composite kernels.
…ove validation function into schema to allow full pedantic check. (3) make temp copies of FIELDS_TO_RESOLVE for composite kernel.
|
This PR aims to increase the number of supported kernels and associated operations.
The objectives of the PR can be summarized as follows:
The work can therefore be divided into the following stages:
Design notes
As inspired by multiple Gaussian Process libraries (i.e. GPy, GPytorch), a base kernel class is the typical implementation for supporting different built-in and user-defined kernel functions.
Concerning alibi-detect, we aim to support the following functionalities.
(1) provides a generalised template for all kernels. (initialisation, compute kernel value).
(2) gives unified management for relevant kernel parameters. (initialisation, inference, training, view and modify).
(3) allows the kernel value to be computed on specific feature dimensions.
While ideally, the above functions should be integrated into a single base class, we have decided to distribute them into separate parts given the current implementation of alibi detect.
On (1), at the moment, the base kernel class is kept minimum. It only inherits the backend nn module and provides a holder for kernel parameters
parameter_dict
. This dictionary will be helpful when the kernel has multiple parameters, as we can loop over the dictionary keys to operate on every parameter without explicitly call the name (i.e.sigma
.)On (2), previously, we hard-coded the initialisation, inference and training of the
sigma
parameter within the RBF kernel (and detectors). This behaviour is lifted now as a separate class calledKernelParameter
. This class is implemented as a wrapper over the corresponding backend variable (tf.Varaible
,torch.nn.Parameter
). The class includesinit_fn
,requires_grad
,requires_init
as its attributes. Therefore, each parameter can be separately inspected over these attributes and invoke the corresponding procedures. As a result, any manipulation over kernel parameters can now be moved outside of the kernel implementation and only requires to declare of an instance ofKernelParameter
when the kernel is written. On the other hand, such an implementation maintains the arguments and logistics of the previous RBF kernel therefore would require limited modification over existing detectors.On (3), following the review comments from @ojcobb, coding the selection within the base class (hence each kernel) will result in duplication of codes and increase the complexity for the users to implement customised kernels. The solution now is to write a wrapper kernel
DimensionSelectKernel
that performs the selection before passing it to a given kernel. Following the conviction of GPy, we refer to the selection argument asactive_dims
.