Closes #623 | Add/Update Dataloader MedEV #639
Conversation
patrickamadeus
requested review from
holylovenia,
SamuelCahyawijaya,
sabilmakbar,
jamesjaya,
yongzx,
gentaiscool,
ljvmiranda921,
jensan-1,
danjohnvelasco,
MJonibek and
tellarin
as code owners
holylovenia
requested review from
akhdanfadh and
elyanah-aco
and removed request for
tellarin,
gentaiscool,
jamesjaya,
SamuelCahyawijaya,
ljvmiranda921,
holylovenia,
yongzx,
MJonibek,
danjohnvelasco,
jensan-1 and
sabilmakbar
There was a problem hiding this comment.
Thanks for the PR @patrickamadeus! Just suggesting some small changes:
seacrowd/sea_datasets/medev/medev.py
Outdated
| """Yields examples as (key, example) tuples.""" | ||
| with open(filepath["en"], "r") as f: | ||
| en_lines = f.readlines() | ||
| with open(filepath["vie"], "r") as f: |
There was a problem hiding this comment.
Can you determine what encoding your computer uses for the Vietnamese file? Both utf-8 and latin-1 don't give me the intended result, and other encodings don't work at all.
There was a problem hiding this comment.
Mine is utf-8. it's working well from my side.
seacrowd/sea_datasets/medev/medev.py
Outdated
| { | ||
| "text": datasets.Value("string"), | ||
| } |
There was a problem hiding this comment.
| { | |
| "text": datasets.Value("string"), | |
| } | |
| { | |
| "id": datasets.Value("string"), | |
| "text": datasets.Value("string"), | |
| } |
There was a problem hiding this comment.
Also adding to this, do we really want to not match the English text and Vietnamese translation together? I know that the dataset viewer in the homepage shows the data in a stack, but I think for a dataloader, we should add them together. Wdyt @elyanah-aco?
There was a problem hiding this comment.
I agree that would be more useful. Maybe something like id, vie_text and eng_text fields for source is okay
There was a problem hiding this comment.
Hi! I'll address @akhdanfadh 's suggestion after everything concluded from ur guys' side!
seacrowd/sea_datasets/medev/medev.py
Outdated
| { | ||
| "text": datasets.Value("string"), | ||
| } |
There was a problem hiding this comment.
Also adding to this, do we really want to not match the English text and Vietnamese translation together? I know that the dataset viewer in the homepage shows the data in a stack, but I think for a dataloader, we should add them together. Wdyt @elyanah-aco?
Co-authored-by: Elyanah Aco <[email protected]>
Co-authored-by: Elyanah Aco <[email protected]>
|
Hi @elyanah-aco ! I've addressed all of the suggestions! Appreciate the detailed review. I will address suggestion from @akhdanfadh after your second opinion 🙏. |
A friendly reminder for @elyanah-aco in case she missed it. |
There was a problem hiding this comment.
Thanks for the changes @patrickamadeus! OK now on my end (even the Vietnamese text)
seacrowd/sea_datasets/medev/medev.py
Outdated
| { | ||
| "text": datasets.Value("string"), | ||
| } |
There was a problem hiding this comment.
I agree that would be more useful. Maybe something like id, vie_text and eng_text fields for source is okay
|
Hi all @akhdanfadh @elyanah-aco ! The minor language expand is done! Thank you for all of the reviews. 🙏 |
|
Hi @akhdanfadh, I would like to let you know that we plan to finalize the calculation of the open contributions (e.g., dataloader implementations) in 31 hours, so it'd be great if we could wrap up the reviewing and merge this PR before then. cc: @patrickamadeus |
Closes #623
Checkbox
seacrowd/sea_datasets/{my_dataset}/{my_dataset}.py(please use only lowercase and underscore for dataset folder naming, as mentioned in dataset issue) and its__init__.pywithin{my_dataset}folder._CITATION,_DATASETNAME,_DESCRIPTION,_HOMEPAGE,_LICENSE,_LOCAL,_URLs,_SUPPORTED_TASKS,_SOURCE_VERSION, and_SEACROWD_VERSIONvariables._info(),_split_generators()and_generate_examples()in dataloader script.BUILDER_CONFIGSclass attribute is a list with at least oneSEACrowdConfigfor the source schema and one for a seacrowd schema.datasets.load_datasetfunction.python -m tests.test_seacrowd seacrowd/sea_datasets/<my_dataset>/<my_dataset>.pyorpython -m tests.test_seacrowd seacrowd/sea_datasets/<my_dataset>/<my_dataset>.py --subset_id {subset_name_without_source_or_seacrowd_suffix}.Tests