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

Squeeze and Excite block #505

Merged
merged 15 commits into from
Jul 7, 2022
Merged

Conversation

AdityaKane2001
Copy link
Contributor

Added SE Block. As discussed, this one can be done after #480 and #499 are merged.

/auto Closes #167

@AdityaKane2001 AdityaKane2001 marked this pull request as ready for review June 25, 2022 09:04
@AdityaKane2001
Copy link
Contributor Author

@sayakpaul @LukeWood

Marking this as ready to review as #480 is now merged.

@sayakpaul
Copy link
Contributor

@LukeWood this needs to have the GSoC2022 label.

keras_cv/layers/regularization/squeeze_excite.py Outdated Show resolved Hide resolved
keras_cv/layers/regularization/squeeze_excite.py Outdated Show resolved Hide resolved
keras_cv/layers/regularization/squeeze_excite_test.py Outdated Show resolved Hide resolved
@AdityaKane2001
Copy link
Contributor Author

@sayakpaul @LukeWood

I have made the changes, please review again.

Copy link
Contributor

@sayakpaul sayakpaul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM given the linting tests pass!

@LukeWood LukeWood mentioned this pull request Jun 28, 2022
Copy link
Contributor

@LukeWood LukeWood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor changes, thanks!

keras_cv/layers/regularization/squeeze_excite.py Outdated Show resolved Hide resolved
keras_cv/layers/regularization/squeeze_excite.py Outdated Show resolved Hide resolved
Copy link
Contributor

@LukeWood LukeWood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one comment then LGTM

keras_cv/layers/regularization/squeeze_excite_test.py Outdated Show resolved Hide resolved
Copy link
Contributor

@LukeWood LukeWood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

last round of comments

keras_cv/layers/regularization/squeeze_excite.py Outdated Show resolved Hide resolved
@AdityaKane2001
Copy link
Contributor Author

AdityaKane2001 commented Jun 30, 2022

@LukeWood I have a workaround for the failing tests:

def custom_compare(obj1, obj2):
    if isinstance(obj1, (core.FactorSampler, tf.keras.layers.Layer)):
        return config_equals(obj1.get_config(), obj2.get_config())
    elif inspect.isfunction(obj1):
        return tf.keras.activations.serialize(obj1) == obj2
    elif inspect.isfunction(obj2):
        return obj1 == tf.keras.activations.serialize(obj2)
    else:
        return obj1 == obj2

The tests were failing when the passed object is a function. So, we check if it is a function and serialize it if it is. I'll replace activations.serialize by keras.utils.serialize_keras_object.

@LukeWood
Copy link
Contributor

@LukeWood I have a workaround for the failing tests:

def custom_compare(obj1, obj2):
    if isinstance(obj1, (core.FactorSampler, tf.keras.layers.Layer)):
        return config_equals(obj1.get_config(), obj2.get_config())
    elif inspect.isfunction(obj1):
        return tf.keras.activations.serialize(obj1) == obj2
    elif inspect.isfunction(obj2):
        return obj1 == tf.keras.activations.serialize(obj2)
    else:
        return obj1 == obj2

The tests were failing when the passed object is a function. So, we check if it is a function and serialize it if it is. I'll replace activations.serialize by keras.utils.serialize_keras_object.

Awesome! Thanks for investigating this. Let’s update so the tests pass!

@AdityaKane2001
Copy link
Contributor Author

@LukeWood Done.

@AdityaKane2001
Copy link
Contributor Author

@sayakpaul @LukeWood

Even though this is a valid workaround at the moment, it would be interesting to see if there is any concrete solution to serializing and deserializing of functions. WDYT? Or is there some existing solution to this?

@AdityaKane2001
Copy link
Contributor Author

@sayakpaul @LukeWood

Could you please take a look at this one when you get a moment? TIA

@AdityaKane2001
Copy link
Contributor Author

@sayakpaul @LukeWood

Could you please take a look at this one? Please let me know if there's anything you'd like me to change.

@LukeWood
Copy link
Contributor

LukeWood commented Jul 7, 2022

Thank you for the PR Aditya, merged

@LukeWood LukeWood merged commit d30d902 into keras-team:master Jul 7, 2022
ianstenbit pushed a commit to ianstenbit/keras-cv that referenced this pull request Aug 6, 2022
* Added SE block

* Formatted

* Made requested changes

* Final touches to SqueezeAndExcite

* Final touches to SqueezeAndExcite

* Made requested changes

* Formatted

* Added activation arguments

* Made requested changes

* Serialization tests workaround

* Made requested changes
adhadse pushed a commit to adhadse/keras-cv that referenced this pull request Sep 17, 2022
* Added SE block

* Formatted

* Made requested changes

* Final touches to SqueezeAndExcite

* Final touches to SqueezeAndExcite

* Made requested changes

* Formatted

* Added activation arguments

* Made requested changes

* Serialization tests workaround

* Made requested changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Squeeze and Excitement Block
4 participants