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

Implement check to skip value range transform #198

Merged
merged 3 commits into from
Mar 23, 2022

Conversation

LukeWood
Copy link
Contributor

No description provided.

@qlzh727 qlzh727 self-requested a review March 22, 2022 21:13
Copy link
Member

@qlzh727 qlzh727 left a comment

Choose a reason for hiding this comment

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

Probably add a simple test case, just for coverage.

@LukeWood
Copy link
Contributor Author

Probably add a simple test case, just for coverage.

Yeah, I was wondering about that. Any suggestion @qlzh727? We’d have to inspect the graph, right?

@LukeWood LukeWood merged commit 569df79 into master Mar 23, 2022
@bhack
Copy link
Contributor

bhack commented Mar 23, 2022

@qlzh727 How is this working between the preprocessing layers chain?

Are we going to have some important casting overhead as we have profiled at #165 (comment)?

@qlzh727
Copy link
Member

qlzh727 commented Mar 23, 2022

if the value range for all the image are (0, 255) for the input image, then it will benefit from this change (since it skip the conversion). When the input range are (0, 1), then any chain of the layer will keep converting them between (0, 1) and (0, 255). We might want to add some warning to user if this method has been hit several times in the pipeline, and suggest user to pre-convert their value to (0, 255).

Anyway, this method is always good to have since it always skip the unnecessary conversion.

@bhack
Copy link
Contributor

bhack commented Mar 23, 2022

We might want to add some warning to user if this method has been hit several times in the pipeline, and suggest user to pre-convert their value to (0, 255).

Can we find a way to communicate/propagate between KLP layers that the range is converted?

@bhack
Copy link
Contributor

bhack commented Mar 23, 2022

As I suppose that a chain of transformations is a quite common use case and it could be the O() of the casting overhead.

@LukeWood LukeWood deleted the fix_transform_value_range branch March 31, 2022 18:40
ianstenbit pushed a commit to ianstenbit/keras-cv that referenced this pull request Aug 6, 2022
* Implement check to skip value range transform

* add simple test case

* Run format
adhadse pushed a commit to adhadse/keras-cv that referenced this pull request Sep 17, 2022
* Implement check to skip value range transform

* add simple test case

* Run format
freedomtan pushed a commit to freedomtan/keras-cv that referenced this pull request Jul 20, 2023
int64 is not a supported type in jax bye default. Trying to use it gets
the following error.

UserWarning: Explicitly requested dtype int64 requested in array is not available, and will be truncated to dtype int32. To enable more dtypes, set the jax_enable_x64 configuration option or the JAX_ENABLE_X64 shell environment variable. See https://github.com/google/jax#current-gotchas for more.
  target = jnp.array(target, dtype="int64")

We can stick to int32 (which is what will be used anyway).
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

Successfully merging this pull request may close these issues.

3 participants