rebase for checking the coverage tests

[mducoffe]

No description provided.

[bartvm]

Just FYI, no need to open a new PR after rebasing. After you've rebased you can force push to your remote by using push -f origin CCW_Transformer.

[mducoffe]

@bartvm I have checked the coverage, the method get_example in Transformer was never tested because Batch was calling get_data all the time instead of get_data_from_example.
I have a question also : is Cache use example stream only or batch stream is okay ?

[rizar]

@bartvm , are the methods get_data_from_example and get_data_from_batch a part of the plan? I do not remember those being discussed.

[bartvm]

Ah, this is something I discussed with @mducoffe by e-mail. Let me just copy-paste the rationale. They're really just renames of the methods we had for these reasons:

I've made some comments, but I realised that the naming is awkward though... Right now get_example and get_batch refer to what the output is (as their name suggests) but the difference between get_example and get_batch should really depend on what the input is. Technically speaking you could have a transformer that can take either an example or a batch as an input, but will output a batch in both cases. The methods will still need to be different though.

A very artificial example, but consider a transformer which outputs batches of size N. Its behaviour changes depending on the input, but the output is always the same:

class BatchOfNExamples(Transformer):
    def get_data_from_batch(self):
        batch = []
        while len(batch) < N:
            batch += next(self.child_epoch_iterator)[:N - len(batch)]
        return batch

    def get_data_from_example(self):
        return [next(self.child_epoch_iterator) for _ in range(N)]

Maybe a good idea would then be to rename get_batch to get_data_from_batch and get_example to get_data_from_example (means you'll have to swap them around for Unpack and Batch), and change the keyword argument from batch to batch_input to make clear that it refers to what the input is, not to what the output is. (I hope you're familiar with your text editor's renaming function...)

[rizar]

I should say this is getting quite involved, which names with 3 underscores kind of suggest. I have to run now, but I will keep thinking how to make things simpler here.

[bartvm]

It's really exactly the same as it was! It's just that get_batch has been renamed to get_data_from_batch to make clear that get_batch doesn't return a batch, but that it expects a batch as input. Another naming suggestion is welcome, but I find things like get_from_batch, from_batch unclear.

[rizar]

It is tempting to propose transform_batch and transform_example that look nice with the Transformer name, but those do not reflect that multiple input entries can make one output entry and vice versa.

I do not mind merging it as it to keep workflow fast, but I will create an issue because such names do not really invite to quickly write your own Transformer.

[mducoffe]

@bartvm done =)

[bartvm]

Python actually automatically concatenates strings within parentheses, so you can do:

raise NotImplementedError(
    "`{}` does not support examples as inputs, "
    "but `batch_input` was set to `False`".format(type(self))
)

[mducoffe]

@bartvm Is it okay for merging ?

[bartvm]

Sorry, I got really confused at one point because I made comments in the old PR and didn't think you addressed them, but looks like you did in this PR... There's some other stuff that seems to have reverted to an old version though :/

[bartvm]

Not sure how this got reintroduced, but we now have batch_input and batch in the docstring.

[bartvm]

After that, ping me and I'll press the merge button straight away, I promise!

[mducoffe]

@bartvm =)