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

Basis refactor pr1 #273

Merged
merged 114 commits into from
Dec 11, 2024
Merged

Basis refactor pr1 #273

merged 114 commits into from
Dec 11, 2024

Conversation

BalzaniEdoardo
Copy link
Collaborator

Basis API Refactor

Overview

This is a massive PR that revisiting the basis API. In particular it addresses #266 and notably breaks backwards compatibility.

The goal was to simplify the initialization and control logic of the basis class. Before this PR the classes for basis could operate in two modes: convolving the input with basis elements, or maps non-linearly the input through the basis, in what was originally mode="conv" and mode="eval" respectively. The issue with this approach is that the initialization presented a number of parameters that only applied to one modality (bounds applies to "eval", "window_size" to "conv").

To design our classes we had to operate under the following constraints:

  1. scikit-learn API: this means that all the retrievable and settable attributes must be defined at initialization for get_params and set_params to work. The name of the attribute must match the name of the parameter defined at initialization.
  2. Basis composition: all basis must have a single compute_features method, so that we can guarantee that complex basis (addition and multiplication of a number of bases) can call recursively the compute_features method of the components. Also, this makes it easier to map a basis to a scikit-learn transformer.

These requirements are incompatible with the original plan of having a single class with default mode of operation, and a method for switching mode. The reason is that if the default initialization requires only a subset of the parameters, the other mode of operation will not respect the scikit-learn API (having parameter set outside the initialization).

We settled for 2 distinct classes for each operational mode, at the cost of doubling the size of the basis API. The pros of this approach is: simpler initialization, and improved modularity of the implementation.

Module Refactor Details

The basis module has been moved from to a folder with the following scripts:

  • _basis.py: contains the Basis abstract class, AdditiveBasis and MultiplicativeBasis (classes that cannot/should not be initialized directly).
  • _basis_mixin.py: contains ConvBasisMixin and EvalBasisMixin, implementing all mode specific methods and the parameter control logic. BasisTransformerMixin equips basis with the to_transformer method. These classes cannot be initialized on their own but add the mode of operation and extra methods to concrete sublcasses of Basis.
  • _transformer_basis.py: Implements the transformer API for basis.
  • basis.py: contains all the concrete basis for both modality. Each basis name starts with either Conv and Eval depending on the mixin super-class.
    _ _raised_cosine_basis.py, _decaying_exponential.py, _spline_basis.py implements the __call__ method and check logic that is specific to each basis type.

@BalzaniEdoardo BalzaniEdoardo mentioned this pull request Dec 4, 2024
Copy link
Member

@billbrod billbrod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Boy this is a big PR. I think we covered most of the content / architectures questions as we've discussed this in person, but I have some tweaks throughout.

Additionally, I think, bigger picture, we will need to rework developer notes.

  • There are two different variants of them: how to write a nemos-compliant basis for your own use, and how we write new basis objects to include in nemos. Those might both belong in the docs, or not (maybe one of them is in contributing?).
  • The developer notes docs should contain a minimum working example of whatever we explain (to serve as a test as well).
  • We also should probably write up "how to do a basis mixin"

I think this is a separate PR though -- should I paste this in #274? or would it be a new one?

docs/api_reference.rst Outdated Show resolved Hide resolved
docs/background/plot_01_1D_basis_function.md Outdated Show resolved Hide resolved
docs/background/plot_01_1D_basis_function.md Outdated Show resolved Hide resolved
docs/background/plot_03_1D_convolution.md Outdated Show resolved Hide resolved
docs/developers_notes/04-basis_module.md Outdated Show resolved Hide resolved
src/nemos/basis/_basis_mixin.py Outdated Show resolved Hide resolved
src/nemos/basis/_basis_mixin.py Show resolved Hide resolved
src/nemos/basis/basis.py Outdated Show resolved Hide resolved
src/nemos/basis/basis.py Outdated Show resolved Hide resolved
src/nemos/basis/_basis.py Outdated Show resolved Hide resolved
BalzaniEdoardo and others added 14 commits December 9, 2024 16:28
Co-authored-by: William F. Broderick <[email protected]>
Co-authored-by: William F. Broderick <[email protected]>
Co-authored-by: William F. Broderick <[email protected]>
Co-authored-by: William F. Broderick <[email protected]>
Co-authored-by: William F. Broderick <[email protected]>
@BalzaniEdoardo
Copy link
Collaborator Author

Boy this is a big PR. I think we covered most of the content / architectures questions as we've discussed this in person, but I have some tweaks throughout.

Additionally, I think, bigger picture, we will need to rework developer notes.

* There are two different variants of them: how to write a nemos-compliant basis for your own use, and how _we_ write new basis objects to include in nemos. Those might both belong in the docs, or not (maybe one of them is in contributing?).

* The developer notes docs should contain a minimum working example of whatever we explain (to serve as a test as well).

* We also should probably write up "how to do a basis mixin"

I think this is a separate PR though -- should I paste this in #274? or would it be a new one?

I think a separate PR, because i am still editing the basis module in the following PRs... I would do fix the notes once the module is more stable

@BalzaniEdoardo
Copy link
Collaborator Author

@billbrod I should have gone through everything. You should ignore the documentation for this PR, and focus just on the code, since in the next PR I'll focus on the documentation. I plan to merge this by the end of the day if there are no more major concerns

Copy link
Member

@billbrod billbrod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double-check that the changes I just pushed look good, but this looks good to me!

@BalzaniEdoardo BalzaniEdoardo merged commit 74bde2b into development Dec 11, 2024
13 checks passed
@BalzaniEdoardo
Copy link
Collaborator Author

Great, thanks Billy!

@BalzaniEdoardo BalzaniEdoardo deleted the basis_refactor_pr1 branch December 11, 2024 14:36
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

Successfully merging this pull request may close these issues.

4 participants