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

Abstract type for the models #183

Open
theabhirath opened this issue Jul 15, 2022 · 5 comments
Open

Abstract type for the models #183

theabhirath opened this issue Jul 15, 2022 · 5 comments

Comments

@theabhirath
Copy link
Member

Given that all the high-level model APIs implement (m::Model)(x), backbone(m::Model) and classifier(m::Model), would it make sense to have an abstract type MetalheadModel that the models can subtype? I take it that there's no way to implement an interface for these functions that can be inherited via types but this way at least we can document in the documentation for MetalheadModel what every single model in the repo implements

@theabhirath
Copy link
Member Author

@darsnack @ToucheSir what do you make of this proposal? This might reduce code duplication if we could write the methods for just the MetalheadModel type (not to mention that this would make the docstrings for backbone and classifier a lot less ambiguous than just saying "use the camel-cased model names"). If FluxML/Flux.jl#1932 landed with Flux v0.13.6 this could go even further and remove the entire pretty-printing mechanism by simply @layering the supertype

@darsnack
Copy link
Member

darsnack commented Sep 7, 2022

I think having an abstract type that's used internally to remove duplication is fine (though I think you really only remove the forward pass and maybe @functor?). What I want to avoid is any notion that a user needs to inherit from this type for their own customized models.

@theabhirath
Copy link
Member Author

though I think you really only remove the forward pass and maybe @functor?

Also backbone and classifier.

What I want to avoid is any notion that a user needs to inherit from this type for their own customized models.

Yeah I think that we can state that explicitly in the docs, shouldn't be a problem. I'll wait on FluxML/Flux.jl#1932 to be resolved before I get this in I think

@theabhirath
Copy link
Member Author

Funny, I can't seem to remove @functor when I try this. It errors by saying type does not have a definite number of fields?

@darsnack
Copy link
Member

darsnack commented Sep 8, 2022

How are you trying to do it? By using the macro? That won't work. You need to define the function functor for the abstract type.

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