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

Add inchannels, imsize, nclasses as kwargs for all constructors #176

Open
ablaom opened this issue Jun 23, 2022 · 11 comments
Open

Add inchannels, imsize, nclasses as kwargs for all constructors #176

ablaom opened this issue Jun 23, 2022 · 11 comments

Comments

@ablaom
Copy link

ablaom commented Jun 23, 2022

I'm looking at Metalhead integration in MLJFlux. To do this well, I'm looking for some uniformity in the Metalhead.jl API that seems to be lacking. In particular, it would help if nclasses and inchannels were supported kwargs for all Metalhead constructors (VGG, and so forth). Perhaps if there are models that really can't support them, an informative error could be thrown?

For example, I think the following ought to just work but don't:

VGG(inchannels=1)
ResNet(inchannels=1)

Also helpful would be a trait to label those constructors that build fixed image size models. I think there's at least one.

@ablaom ablaom changed the title [Feature request] Add inchannels and nclasses as kwarg for all constructors Non-breaking improvement to the API: Add inchannels and nclasses as kwargs for all constructors Jun 23, 2022
@theabhirath
Copy link
Member

theabhirath commented Jun 23, 2022

nclasses is supported by every current Metalhead model, I believe. inchannels isn't but I have been working on trying to unify the API a little bit (see #174 for ResNet, for example), and so it can be easily done as part of that.

Also helpful would be a trait to label those constructors that build fixed image size models. I think there's at least one.

This might be a little complicated. Some models explicitly allow for an imsize parameter to be passed in. And while others don't, that does not necessarily mean they don't work. The Inception series is a good example - while being designed for 299x299 RGB images, they also sometimes end up working with lower resolutions. I'll see if there's a way I can ascertain this for sure

@ablaom
Copy link
Author

ablaom commented Jun 23, 2022

Thanks for the lightning feedback, @theabhirath.

nclasses is supported by every current Metalhead model, I believe. inchannels isn't but I have been working on trying to unify the API a little bit (see #174 for ResNet, for example), and so it can be easily done as part of that.

👍🏾

This might be a little complicated.

In my use-case an imsize is always available. Perhaps all models also accept imsize, but throw error/warning if that choice of imsize is definitely/possilby going to lead to unexpected behaviour.

@darsnack
Copy link
Member

I think we can support image size, input channels, and output classes as consistent arguments to the highest level constructors (i.e. the ones that have the pretrain keyword). I think this should be good for MLJ, since you plan to support standard architectures with tweaks on the input/output side only?

We can probably have a non-breaking release that adds the interface for this, but not all the models might fully support all the knobs. Once @theabhirath's work lands though, I think we'll be able to support it for every CNN-like architecture.

@ablaom
Copy link
Author

ablaom commented Jun 23, 2022

Sounds good. Be good if there is a way for me to test if a given constructor will support this extended API, however. Save me having to explicitly document which constructors are allowed for use in MLJFlux and update later.

@ablaom ablaom changed the title Non-breaking improvement to the API: Add inchannels and nclasses as kwargs for all constructors Add inchannels, imsize, nclasses` as kwargs for all constructors Jun 27, 2022
@ablaom ablaom changed the title Add inchannels, imsize, nclasses` as kwargs for all constructors Add inchannels, imsize, nclasses as kwargs for all constructors Jun 27, 2022
@ablaom
Copy link
Author

ablaom commented Feb 8, 2023

Has there been any recent progress on this issue?

@darsnack
Copy link
Member

darsnack commented Feb 8, 2023

I see two things outstanding: (1) imsize support and (2) testing whether a model allows certain options. We can easily do (1) and throw a warning when the model does not allow this kind of configuration. Is (2) required if you know a priori that every model in Metalhead has these switches?

Other issue is that we have not made a release in a long time (which is keeping these updates from reaching you). (I'll look into this part later today).

@ablaom
Copy link
Author

ablaom commented Feb 8, 2023

@darsnack Thanks for the update and speed response.

Is (2) required if you know a priori that every model in Metalhead has these switches?

No.

@ablaom
Copy link
Author

ablaom commented Jun 18, 2023

I see 0.8 has been released. But I take it this is still open?

@theabhirath
Copy link
Member

inchannels and nclasses have been added as kwargs to all the models, I believe. But imsize is not so straightforward, so support for that has not yet landed for all of the models (it is there for quite a good number of them though).

@IanButterworth
Copy link
Contributor

IanButterworth commented Dec 21, 2023

I'm not sure if this warrants another issue but the docstrings for models without imsize also don't say what the expected imsize is.

@theabhirath
Copy link
Member

I'm not sure if this warrants another issue but the docstrings for models without imsize also don't say what the expected imsize is.

Oops, I'll try and work on a docs PR to solve this

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

4 participants