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

Replaced resample function by new implementation (2) #1412

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

AlexFuster
Copy link

@AlexFuster
Copy link
Author

I don't get it, I run
python tools/lint/black_python.py --check tensorflow_io/core/python/ops/io_tensor_ops.py in my environment and works perfectly without error

@yongtang
Copy link
Member

yongtang commented May 11, 2021

@AlexFuster if you have black installed, can you just run black audio_ops.py and see if the file is formatted automatically?

@AlexFuster
Copy link
Author

done

@AlexFuster
Copy link
Author

AlexFuster commented May 11, 2021

The tests seem to be taking too long. At least in macOS and linux

@yongtang
Copy link
Member

@AlexFuster Can you rebase the PR with the latest master branch? We recently made some enhancement on testing which I think will resolve some issues shown in GitHub Actions CI.

@AlexFuster
Copy link
Author

ok, here we go again

@AlexFuster
Copy link
Author

Oh yeah, I forgot to mention It does just support floating point audios. Is this a problem?

@yongtang
Copy link
Member

Oh yeah, I forgot to mention It does just support floating point audios. Is this a problem?

@AlexFuster The existing implementation support both int16 and float32. Though I think it will not be too difficult to add int16 support by casting to float32 and back.

@yongtang
Copy link
Member

@AlexFuster The test failures are legitimate. The reason being

  1. Tensor in tensorflow may not be treated as python native value (e.g, 1, 1.1, etc). Things like int(x) may not work when x is a tensor. Instead, an explicit op of tf.cast(x, tf.int32) is needed.
  2. A tensor's shape may not be known statically before hand. So x.shape could just return None in case shape is not known yet.
  3. When x is a tensor, the if...else... logic does not work as tensor's value is not statically available (before run). Instead a TF specific op of tf.case or tf.cond has to be used (you might notice the original implementation consists of tf.case to replace if...elif...else.

You may want to take a look at the test failures and see if some change is needed. I will also take a look.

@AlexFuster
Copy link
Author

I don't think the error comes from any of those 3 problems. It is just tf.nn.conv1d not accepting int16 values as inputs. I'll try adding int16 support by casting and casting back as you said

@yongtang
Copy link
Member

With int16 tests may pass, though in the original implementation the resample actually support passing rate_in, rate_out as tensor and input with unknown shape. I think with new implementation this ability is removed as I could see gcd and int(rate_in) will cause the failure if a tensor with unknown rank is passed for rate_in. The input may also cause some issue if it is a tensor with unknown shape. We can get the tests pass and address the rest in follow up PRs though.

@AlexFuster
Copy link
Author

Why would you want a tensor with a shape other than () being passed as rate_in?

@yongtang
Copy link
Member

Why would you want a tensor with a shape other than () being passed as rate_in?
@AlexFuster Sorry it should be a tensor without constant value. In graph mode (such as within tf.data.Dataset pipeline), user may pass input, rate_in, rate_out as tensor without const value. In that case, the graph mode will fail. They can be avoided by using ops that will work for tensor. For example, int(rate_in) can be converted to use tf.cast(rate_in, tf.int32), math.gcd can be replaced with ops as tf.experimental.numpy.gcd I think.

@google-cla
Copy link

google-cla bot commented Jun 11, 2021

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

@AlexFuster
Copy link
Author

@googlebot I fixed it.

@AlexFuster
Copy link
Author

AlexFuster commented Jun 16, 2021

ok, definitely the problem is in tf.nn.conv1d not accepting a tensor as a value for stride in graph mode (it does in eager mode). Any idea?

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.

4 participants