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

Denormalization layer #16350

Merged
merged 5 commits into from
Apr 8, 2022
Merged

Conversation

markub3327
Copy link
Contributor

@markub3327 markub3327 commented Apr 2, 2022

@fchollet
Copy link
Member

fchollet commented Apr 2, 2022

Thanks for the PR. I see we have an invert argument in the preprocessing layers StringLookup and
IntegerLookup. Perhaps this use case could be covered in a simpler fashion by also adding invert to the Normalization layer?

@mattdangerw what do you think?

@gbaned gbaned requested a review from fchollet April 4, 2022 15:12
@google-ml-butler google-ml-butler bot added the keras-team-review-pending Pending review by a Keras team member. label Apr 4, 2022
@markub3327
Copy link
Contributor Author

markub3327 commented Apr 4, 2022

@fchollet
I changed it to version with invert=False. I think it's better solution too.

Please make a review.

@mattdangerw
Copy link
Member

Yeah, I think invert as an argument make sense. Could you add a unit test seeing if this would also be workable for an adapted layer? I think the following snippet should work, but it would be good to check.

norm = keras.layers.Normalization(axis=None)
norm.adapt(some_data)
inv_norm = keras.layers.Normalization(
    mean=norm.mean,
    variance=norm.variance,
    axis=norm.axis,
    invert=True)

We should probably also show an example of this in the docstring, taking some data, first normalizing, then inverting. For both the adapted and non-adapted cases.

@markub3327
Copy link
Contributor Author

@mattdangerw

I create both unit tests.

Copy link
Member

@fchollet fchollet left a comment

Choose a reason for hiding this comment

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

LGTM, thank you

@google-ml-butler google-ml-butler bot added kokoro:force-run ready to pull Ready to be merged into the codebase labels Apr 5, 2022
@mattdangerw
Copy link
Member

@markub3327 thanks. Could you add an example of invert in the docstring as well?

@markub3327
Copy link
Contributor Author

markub3327 commented Apr 6, 2022

@mattdangerw

Yes, sorry I forgotten. It's done.

@google-ml-butler google-ml-butler bot removed the ready to pull Ready to be merged into the codebase label Apr 6, 2022
Copy link
Member

@fchollet fchollet left a comment

Choose a reason for hiding this comment

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

LGTM, thanks. I'll do a round of copyedits in post-processing.

@google-ml-butler google-ml-butler bot added kokoro:force-run ready to pull Ready to be merged into the codebase labels Apr 6, 2022
@fchollet fchollet removed the keras-team-review-pending Pending review by a Keras team member. label Apr 7, 2022
@markub3327
Copy link
Contributor Author

@fchollet

Ok. Thanks.

@copybara-service copybara-service bot merged commit 4c87dc9 into keras-team:master Apr 8, 2022
@markub3327 markub3327 deleted the master2 branch April 9, 2022 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to pull Ready to be merged into the codebase size:L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants