search

paganpasta / eqxvision

A Python package of computer vision models for the Equinox ecosystem.
https://eqxvision.readthedocs.io
MIT License
100 stars 12 forks source link

Add VGG16 with basic image preprocessing #10

Closed oarriaga closed 2 years ago

paganpasta commented 2 years ago

Can we make the implementation general for the VGG family? What I mean is VGG16 is quite a specific model and others like VGG11(+BN), VGG19(+BN) etc can also be supported with minimal effort.

paganpasta commented 2 years ago

Just a reminder for later stages, missing test cases to verify that forward pass is going through with and without jit.

oarriaga commented 2 years ago

Unfortunately, at the moment I don't have much time to think about how one could make it general for other VGG's. But after this PR someone with more time can take over that task. Moreover, I think that independent code bases between VGG16 or VGG19 might be better, rather than abstracting some small generalizations. For example VGG16 and VGG19 model implementations in Keras are independent of each other, and this makes it easy and simple to read and understand. What do you think?

paganpasta commented 2 years ago

The issue I raised this is because existing model architectures in eqxvision support model_* variations and just supporting VGG16 for VGG will break the consistency for the rest. Plus, having multiple classes declared (with/wo BatchNorm) also increases duplicacy of the code. Don't worry about the time I can take it over from here for this else if you still wish to push for it then you are more than welcome!

paganpasta commented 2 years ago

@oarriaga Sorry, just wanted to confirm, should I close the merge? I can bundle in VGG with test case revamp I was doing.

oarriaga commented 2 years ago

Ah OK I was thinking that you could accept the PR and continue developing with another PR or just commits to master. But I see now that you continued it with your own commits.

paganpasta commented 2 years ago

Sorry for the confusion.