SEACrowd / seacrowd-datahub

A collaborative project to collect datasets in SEA languages, SEA regions, or SEA cultures.
Apache License 2.0
60 stars 56 forks source link

Closes #442 | Add dataloader for ASR-MALCSC #494

Closed zwenyu closed 4 months ago

zwenyu commented 6 months ago

Closes #442

Checkbox

elyanah-aco commented 5 months ago

Hi @zwenyu, thanks for the dataloader submission! Dataloader works fine for me, just have some suggestions and comments

zwenyu commented 5 months ago

@elyanah-aco Thanks for the comments. I've pushed changes according to the suggestions. For the returned text, I've removed the timestamps.

ljvmiranda921 commented 5 months ago

Hi @zwenyu , the data loader works for me! So no issues with implementation! As for the timestamps, I think we should include it in the text. Without it, can we still map the audio to the transcription? If not then maybe the timestamps are relevant 🤔 . Thoughts? @elyanah-aco

elyanah-aco commented 5 months ago

@ljvmiranda921 Honestly either is okay for me - I listened to some of the audio files and there's just silence in periods without timestamps. Each utterance is separated by \n right now which I think is enough. But I defer to what option is most useful to researchers at the end of the day, haha

ljvmiranda921 commented 5 months ago

Each utterance is separated by \n right now which I think is enough

Ah I see if that's the case then I think it's all good. LGTM!

holylovenia commented 4 months ago

@zwenyu Could you please add timestamp under metadata in the seacrowd_sptext schema as well? It should be fine as long as the unit test doesn't raise an error.

Added.

Thanks for the addition, @zwenyu!

@elyanah-aco, may I know if this PR is ready for merge?