Skip to content
This repository was archived by the owner on Sep 10, 2025. It is now read-only.

Conversation

@parmeet
Copy link
Contributor

@parmeet parmeet commented Feb 9, 2022

Reference issue: #1493

This PR do couple of things:

  1. Add Mock test similar to IWSLT testing to start from compressed file #1596. Original credit: @erip due to Fix IWSLT2016 testing #1585
  2. Fix an issue on IWSLT2017 that created additional texts folder outside of extracted archive

@parmeet
Copy link
Contributor Author

parmeet commented Feb 9, 2022

@erip would be great if you can also take a look and catch any obvious issues :)

@parmeet parmeet requested a review from Nayef211 February 9, 2022 22:46
@codecov
Copy link

codecov bot commented Feb 10, 2022

Codecov Report

Merging #1598 (ef3ab65) into main (18b61fa) will increase coverage by 2.02%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1598      +/-   ##
==========================================
+ Coverage   82.11%   84.13%   +2.02%     
==========================================
  Files          58       58              
  Lines        2566     2566              
==========================================
+ Hits         2107     2159      +52     
+ Misses        459      407      -52     
Impacted Files Coverage Δ
torchtext/datasets/iwslt2017.py 92.72% <100.00%> (+65.45%) ⬆️
torchtext/data/datasets_utils.py 64.74% <0.00%> (+5.75%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 18b61fa...ef3ab65. Read the comment docs.

Copy link
Contributor

@Nayef211 Nayef211 left a comment

Choose a reason for hiding this comment

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

This PR looks good and seems consistent with the IWSLT2016 test. Since the two tests share so much similarity, do you think it would be feasible to parameterize them into one test class instead?

@parmeet
Copy link
Contributor Author

parmeet commented Feb 10, 2022

This PR looks good and seems consistent with the IWSLT2016 test. Since the two tests share so much similarity, do you think it would be feasible to parameterize them into one test class instead?

hmm, not really. Their compression structures are quite different for us to efficiently merge the _get_mock_dataset function for the two test cases. The common part of them are already in datasets_utils.py.

@parmeet parmeet merged commit c3f59a5 into pytorch:main Feb 10, 2022
@parmeet parmeet deleted the test_iwslt2017 branch February 10, 2022 14:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants