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

Should we separate basis objects' eval and conv mode? #202

Open
billbrod opened this issue Jul 26, 2024 · 3 comments
Open

Should we separate basis objects' eval and conv mode? #202

billbrod opened this issue Jul 26, 2024 · 3 comments

Comments

@billbrod
Copy link
Member

With the changes in #191 , the eval and conv modes of the basis object are becoming more and more separate, such that there a handful of initialization arguments that are ignored depending on which one you choose. This feels weird to me -- should we have them be separate objects somehow? Or initialize with shared arguments and then call a eval() or conv() method to set the mode, passing the relevant arguments then?

@BalzaniEdoardo
Copy link
Collaborator

the power of the current approach is that you can combine and call the same method "compute_features", which will create any combination of convolution or evaluation for you once the parameters are set at initialization.

To achieve the behavior, we need to set configurations that are not passed before the actual transformation takes place. I don't like the idea that a basis is initialized in a default mode and then one needs to call an extra method to finish off the initialization to switch mode and pass hyperparameters.

Having separate classes may be an option, but we have already a number of basis (possibly expanding), i think it would be a lot of api to remember.

The solution we have now is not super elegant, but keeps the api to a minimum, open to suggestion on how to deal with this init more elegantly!

@BalzaniEdoardo
Copy link
Collaborator

Revisiting my opinion:
After adding a utility function for computing slices that applies to the feature axis, I needed to add an extra parameter that is required only for basis in mode="conv", this is an argument to keep the classes separated, I will take care of that in a dedicated PR.

@BalzaniEdoardo
Copy link
Collaborator

After the PR #247, basis will naturally have an equivalent of "fit" (setting the input shapes, and the conv kernels) and. a transform method (creating the features, the private _compute_features), and a "fit_transform" (compute_features) method.

I would make this more explicit when I'll split the PR, and this will make the map to TransformerBasis even cleaner

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

No branches or pull requests

2 participants