-
Notifications
You must be signed in to change notification settings - Fork 235
feat(v2): add audio url and predefined document #940
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
Conversation
Signed-off-by: anna-charlotte <[email protected]>
Signed-off-by: anna-charlotte <[email protected]>
1285a1c to
6025c2f
Compare
Signed-off-by: anna-charlotte <[email protected]>
Signed-off-by: anna-charlotte <[email protected]>
…udio-v2 Signed-off-by: anna-charlotte <[email protected]>
Signed-off-by: anna-charlotte <[email protected]>
Signed-off-by: anna-charlotte <[email protected]>
Signed-off-by: anna-charlotte <[email protected]>
Signed-off-by: anna-charlotte <[email protected]>
Signed-off-by: anna-charlotte <[email protected]>
Signed-off-by: anna-charlotte <[email protected]>
Signed-off-by: anna-charlotte <[email protected]>
Signed-off-by: anna-charlotte <[email protected]>
Signed-off-by: anna-charlotte <[email protected]>
Signed-off-by: anna-charlotte <[email protected]>
Signed-off-by: anna-charlotte <[email protected]>
Signed-off-by: anna-charlotte <[email protected]>
Signed-off-by: anna-charlotte <[email protected]>
Signed-off-by: anna-charlotte <[email protected]>
Signed-off-by: anna-charlotte <[email protected]>
Signed-off-by: anna-charlotte <[email protected]>
|
@JoanFM @alaeddine-13 I checked all the comments and made corresponding changes. It's ready for re-review now. |
Signed-off-by: anna-charlotte <[email protected]>
Signed-off-by: anna-charlotte <[email protected]>
Signed-off-by: anna-charlotte <[email protected]>
docarray/typing/url/audio_url.py
Outdated
| ) | ||
| return cls(str(url), scheme=None) | ||
|
|
||
| def load(self: T) -> np.ndarray: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@samsja Should I return a np.ndarray here, or rather AudioNdArray or AudioTorchTensor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say AudioNdArray.
Normally we just load the np.ndarray, bc the framework specific conversion can happen when putting it back into the document, and having it as np.ndarray makes the least assumptions about the sorrounding code.
In this case, since there are actual audio features that come with the array, I would go with AudioNdArray. It can still be treated like a normal np.ndarray, but bring the aforementioned features.
But just verify in a test that setting a AudioNdArray to a field with type AudioTorchArray actually works without issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But just verify in a test that setting a AudioNdArray to a field with type AudioTorchArray actually works without issue
Do you mean like this?
def test_load_audio_url_to_audio_torch_tensor(file_url):
class MyAudioDoc(Document):
audio_url: AudioUrl
tensor: Optional[AudioTorchTensor]
doc = MyAudioDoc(audio_url=file_url)
doc.tensor = doc.audio_url.load()
assert isinstance(doc.tensor, np.ndarray)
assert isinstance(doc.tensor, AudioNdArray)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I moved it to the computational backends
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great I like it better like this
samsja
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pr looks really nice. I like how the idea of having different tensor types for the different modality work :) I added some comments
JohannesMessner
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great PR, love the audio tensor stuff, this is a good pattern to use moving forward. just some things to consider
docarray/typing/url/audio_url.py
Outdated
| ) | ||
| return cls(str(url), scheme=None) | ||
|
|
||
| def load(self: T) -> np.ndarray: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say AudioNdArray.
Normally we just load the np.ndarray, bc the framework specific conversion can happen when putting it back into the document, and having it as np.ndarray makes the least assumptions about the sorrounding code.
In this case, since there are actual audio features that come with the array, I would go with AudioNdArray. It can still be treated like a normal np.ndarray, but bring the aforementioned features.
But just verify in a test that setting a AudioNdArray to a field with type AudioTorchArray actually works without issue?
Signed-off-by: anna-charlotte <[email protected]>
Signed-off-by: anna-charlotte <[email protected]>
Signed-off-by: anna-charlotte <[email protected]>
Signed-off-by: anna-charlotte <[email protected]>
Signed-off-by: anna-charlotte <[email protected]>
Signed-off-by: anna-charlotte <[email protected]>
JohannesMessner
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just resolve the conflicts and we're good to go, great work!
Signed-off-by: anna-charlotte <[email protected]>
Signed-off-by: anna-charlotte <[email protected]>
Signed-off-by: anna-charlotte <[email protected]>
|
📝 Docs are deployed on https://ft-feat-add-audio-v2--jina-docs.netlify.app 🎉 |
Add support for audio files to docarray v2
Goals:
AudioUrlAudioTensorwithAudioNdarrayandAudioTorchTensorAudiopredefined doc (with optional attrsAudio.tensor,Audio.urlandAudio.embedding)