-
Notifications
You must be signed in to change notification settings - Fork 91
Parametrize offset models #265
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -260,59 +260,62 @@ def num_trainable_parameters(self) -> int: | |||||
| param.numel() for param in self.parameters() if param.requires_grad) | ||||||
|
|
||||||
|
|
||||||
| @register("offset10-model") | ||||||
| class Offset10Model(_OffsetModel, ConvolutionalModelMixin): | ||||||
| """CEBRA model with a 10 sample receptive field.""" | ||||||
| @parametrize("offset{n_offset}-model", | ||||||
| n_offset=(5, 10, 15, 18, 20, 31, 36, 40, 50)) | ||||||
| class OffsetNModel(_OffsetModel, ConvolutionalModelMixin): | ||||||
| """CEBRA model with a `n_offset` sample receptive field. | ||||||
|
|
||||||
| def __init__(self, num_neurons, num_units, num_output, normalize=True): | ||||||
| n_offset: The size of the receptive field. | ||||||
| """ | ||||||
|
|
||||||
| def __init__(self, | ||||||
| num_neurons, | ||||||
| num_units, | ||||||
| num_output, | ||||||
| n_offset, | ||||||
| normalize=True): | ||||||
| if num_units < 1: | ||||||
| raise ValueError( | ||||||
| f"Hidden dimension needs to be at least 1, but got {num_units}." | ||||||
| ) | ||||||
|
|
||||||
| self.n_offset = n_offset | ||||||
|
|
||||||
| def _compute_num_layers(n_offset): | ||||||
| """Compute the number of layers to add on top of the first and last conv layers.""" | ||||||
| return (n_offset - 4) // 2 + self.n_offset % 2 | ||||||
|
|
||||||
| last_layer_kernel = 3 if (self.n_offset % 2) == 0 else 2 | ||||||
| super().__init__( | ||||||
| nn.Conv1d(num_neurons, num_units, 2), | ||||||
| nn.GELU(), | ||||||
| *self._make_layers(num_units, num_layers=3), | ||||||
| nn.Conv1d(num_units, num_output, 3), | ||||||
| *self._make_layers(num_units, | ||||||
| num_layers=_compute_num_layers(self.n_offset)), | ||||||
| nn.Conv1d(num_units, num_output, last_layer_kernel), | ||||||
| num_input=num_neurons, | ||||||
| num_output=num_output, | ||||||
| normalize=normalize, | ||||||
| ) | ||||||
|
|
||||||
| def get_offset(self) -> cebra.data.datatypes.Offset: | ||||||
| """See :py:meth:`~.Model.get_offset`""" | ||||||
| return cebra.data.Offset(5, 5) | ||||||
| """See `:py:meth:Model.get_offset`""" | ||||||
|
||||||
| """See `:py:meth:Model.get_offset`""" | |
| """See :py:meth:`~.Model.get_offset`""" |
Copilot
AI
Jan 15, 2026
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.
There is a naming conflict - Offset10Model is used here for the 'offset10-model-mse-tanh' architecture, but the same class name may conflict with models generated by the parametrize decorator. This class should be renamed to Offset10ModelMSETanh to avoid confusion and match the pattern used in line 775 (Offset0ModelMSETanH).
| class Offset10Model(_OffsetModel, ConvolutionalModelMixin): | |
| class Offset10ModelMSETanh(_OffsetModel, ConvolutionalModelMixin): |
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 function parameter
n_offsetis passed butself.n_offsetis used in the return statement. This creates inconsistency - either use the parameter consistently or remove it. The function should usen_offset(the parameter) instead ofself.n_offsetin the calculation.