fastai / fastai_dev

fast.ai early development experiments
Apache License 2.0
638 stars 350 forks source link

tidy up tests, consistent reference to and not or #293

Closed amaarora closed 4 years ago

amaarora commented 4 years ago

This PR attempts to tidy up tests in TfmdLists. There were a lot of overlapping tests for the same thing, this PR attempts to fix that. Also, updates names to make more sense in the documentation. After spending a lot of time and understanding TfmdLists, few new tests have also been added that would help the user reading the documentation.

As an example:

test_eq(tl.train, tl.subset(0))
test_eq(tl.valid, tl.subset(1))
test_eq(tl.train.items, items[splits[0]])
test_eq(tl.valid.items, items[splits[1]])

Also, in some funcs of TfmdList, obj was referred to as o while elsewhere as x, this PR attempts to fix that for a part of the notebook involving TfmdLists.

If this PR get's merged, I will create a new one, to tidy up tests for dsrc depending on feedback :)

review-notebook-app[bot] commented 4 years ago

Check out this pull request on  ReviewNB

You'll be able to see Jupyter notebook diff and discuss changes. Powered by ReviewNB.

amaarora commented 4 years ago

I have tried to make sure that we do not lose any tests, but assumed that if a test passes once of a tl, the same test will pass again on a diff tl, to reduce overlapping tests for the same thing.

Also,

FilteredBase.train,FilteredBase.valid = add_props(lambda i,x: x.subset(i), 2)

In add_props, n=2 by default there 2 is removed from add_props(lambda i,x: x.subset(i), 2) as its not needed.

Documentation for subset has been updated to "New TfmdList with same tfms that only includes items in ith split".

These are the entirety of changes proposed in this PR.

amaarora commented 4 years ago

Always happy to receive feedback and make updates if needed :)

sgugger commented 4 years ago

Thanks a lot for this clean-up! I just have one comment on names you changed, but other than that it looks good!

amaarora commented 4 years ago

Thanks @sgugger for the feedback

I have updated names of functions and classes used in only tests with _ at the start :)

amaarora commented 4 years ago

PS: I sent you a review request by mistake, it is like the red button I wanted to press to see what it does :)

sgugger commented 4 years ago

Thanks!