Video Swin model adds to kerashub #1981
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
Hi @divyashreepathihalli! I need some guidance on how to test this locally. Could you help me with the steps? |
|
@divyashreepathihalli can you run test again. |
kernel-loophole
changed the title
There was a problem hiding this comment.
Higher level feedback for a start.
- Remove all "aliases," that's a CV pattern we did not continue. All arch configurations are stored in json files we upload to Kaggle/HF. The "aliases" is just the preset name you want to use.
- Follow the task setup we have in KerasHub. I am guessing for this model we want to add a
VideoClassifiertask that we can model heavily onImageClassifier. - Remove all instances of
keras_cvwhereever they are. - Remove the presets stuff until presets are actually upload.
This will need a much more substantial rewrite to match the abstractions of KerasHub, rather than a more direct copy.
|
Thank you for the feedback. I will remove the aliases and replace them with the appropriate configurations stored in JSON files for Kaggle/HF. I'll follow the KerasHub task setup and create the VideoClassifier task based on the ImageClassifier model. |
|
@kernel-loophole are you still working on this? Please let us know once you are done addressing the comments. |
|
@divyashreepathihalli yes ,i was bit busy ,will try to add this weekend |
kernel-loophole
force-pushed
the
master
branch
from
59e5c8e to
98fabab
Compare
|
@mattdangerw update all cv pattern .configuration are in json file .it would be great if you can review and let me know your feedback on this .if you can provide any example on task setup that would also be great . |
There was a problem hiding this comment.
Hi @kernel-loophole the PR would need more updates to match KerasHub model implementation style. please follow this folder here as an example - https://github.com/keras-team/keras-hub/tree/master/keras_hub/src/models/sam
- remove aliases
- some args like rescaling, input_tensor etc have been dropped
- follow docstring formats
- consistent arg names across similar model implementations.
- use standard test routines like in example folder - https://github.com/keras-team/keras-hub/blob/master/keras_hub/src/models/sam/sam_backbone_test.py
| input_tensor (KerasTensor, optional): Output of | ||
| `keras.layers.Input()`) to use as video input for the model. | ||
| Defaults to `None`. | ||
| include_rescaling (bool, optional): Whether to rescale the inputs. If |
There was a problem hiding this comment.
type hints are specified in this format
arg_name: type. short description
please refer other model implementation -ex: https://github.com/keras-team/keras-hub/blob/master/keras_hub/src/models/vit/vit_backbone.py
There was a problem hiding this comment.
should i format all arg_name according to that .
|
@divyashreepathihalli thanks for review .will update that shortly . |
| input_tensor (KerasTensor, optional): Output of | ||
| `keras.layers.Input()`) to use as video input for the model. | ||
| Defaults to `None`. | ||
| include_rescaling (bool, optional): Whether to rescale the inputs. If |
There was a problem hiding this comment.
should i format all arg_name according to that .
|
|
||
| x = input_spec | ||
|
|
||
| # if include_rescaling: |
There was a problem hiding this comment.
by changing the default value of scaling can change the model behavior
|
@kernel-loophole Sorry about the delayed response. can you please sign the CLA? That would help trigger the CPU tests! |
There was a problem hiding this comment.
Hi @kernel-loophole I did a initial pass - did not look into the layer implementations much. They need to be implemented as layers and not models.
In general please refer to other model implementations to get an idea of design patterns and test routines. And implementation style comments are applicable throughout the code.
| return copy.deepcopy(backbone_presets) | ||
|
|
||
| @classproperty | ||
| def presets_with_weights(cls): |
There was a problem hiding this comment.
remove this - not followed in KerasHub style - please refer to other ported models as reference
| return config | ||
|
|
||
|
|
||
| class VideoSwinPatchingAndEmbedding(keras.Model): |
There was a problem hiding this comment.
should be implemented as a layer
| norm_layer : keras.layers. Normalization layer. | ||
| Default: LayerNormalization | ||
|
|
||
| References: |
There was a problem hiding this comment.
add reference as part of description
| return config | ||
|
|
||
|
|
||
| class VideoSwinBasicLayer(keras.Model): |
There was a problem hiding this comment.
rename to VideoSwinTransformerLayer
There was a problem hiding this comment.
and it should subclass a layer and not a model
| drop_rate=0.0, | ||
| attn_drop_rate=0.0, | ||
| drop_path_rate=0.2, | ||
| depths=[2, 2, 6, 2], |
There was a problem hiding this comment.
do not provide default value for args that change model architecture
|
|
||
|
|
||
| class TestVideoSwinPatchingAndEmbedding(TestCase): | ||
| def test_patch_embedding_compute_output_shape(self): |
There was a problem hiding this comment.
use standerdized test routines for layers - refer to other models and layers
| qk_scale : float. Override default qk scale of head_dim ** -0.5 if set. | ||
| Default to None. | ||
| dropout_rate : float. Float between 0 and 1. Fraction of the input units to drop. | ||
| Default: 0. |
| x = input_spec | ||
|
|
||
| # if include_rescaling: | ||
| # # Use common rescaling strategy across keras_cv |
There was a problem hiding this comment.
remove include_rescaling this is moved into image converter
as discussed before
#1755
@divyashreepathihalli