extend DummyDatasetMultipleSequenceLength #492
Conversation
jotix16
force-pushed
the
add_dummy_seq_tags_order
branch
from
c773046 to
f704424
Compare
| """ | ||
| :rtype: list[str] | ||
| """ | ||
| return ["seq-%i" % i for i in range(self._num_seqs)] |
There was a problem hiding this comment.
self._num_seqs might be inf. It should handle that correctly (fall back to the default behavior).
There was a problem hiding this comment.
What is the default behaviour?
Does it even make sense for a dummy dataset to be initialized with num_seqs=inf?
In GeneratingDataset it is per default inf but here it has to be set by the user.
There was a problem hiding this comment.
What is the default behaviour?
Whatever super of this function does. Just check the code.
Does it even make sense for a dummy dataset to be initialized with
num_seqs=inf?
Yes it does. Why not? It's usually only used for testing. In any case, this is explicitly supported.
| def get_seq_len(i): | ||
| return self.seq_len['data'] | ||
|
|
||
| self._seq_order = self.get_seq_order_for_epoch(epoch, self._num_seqs, get_seq_len=get_seq_len) |
There was a problem hiding this comment.
I'm not sure this makes sense to support for this dataset.
Also you are not using it, i.e. ignoring it (but maybe just because it's incomplete so far).
There was a problem hiding this comment.
The self._seq_order you mean?
There was a problem hiding this comment.
By support, I mean init_seq_order in general.
By use, I mean _seq_order.
|
Why is this specific for |
|
Please also see failing tests. |
jotix16
changed the title
I don't see any reason why not to use it for |
Added
get_all_tags,init_seq_orderandget_current_seq_orderfor DummyDatasetMultipleSequenceLength