Re-align GTZAN with other datasets

[vincentqb]

The update done in #791 removed walk_files from GTZAN, and added pattern recognition. We'll need to re-align GTZAN with the other dataset.

I suggest we simply add a filter after walk_files instead of updating walk_files, as done in SPEECHCOMMANDS. As suggested by @mthrok below, let's instead iterate through the folder structure.

[mmxgn]

Hi,

That PR addressed the comment on #764 that with walk_files we could introduce extra files.

2. `GTZAN` dataset does not have the list of files that corresponds to the entire dataset. Since `GTZAN` has a fixed number of samples, it should not be traversing all the audio files under the given directory. This could introduce extra files or does not react to missing files.
   @mmxgn Do you have a thought on 2.?

I could still change it to use walk_files and do the pattern recognition with filter if that's ok?

[vincentqb]

I could still change it to use walk_files and do the pattern recognition with filter if that's ok?

That would be great, thanks!

[mthrok]

I do not think it's a good idea to keep using walk_files. The utility is solving the problem that Dataset does not have. Once the root directory of each dataset is located, then there is no need to do directory traversal recursively. It's better to have a new utility function that does exactly what is needed by Dataset requirements, that is iterate the audio files in the list of subdirectories in dataset.

The gist is;

for subdir in subdirs:
    for audio_file in subdir:
        yield audio_file

Keep using walk_files forces Dataset classes to define the problem in loose manner (unnecessary recursion) and neither of Dataset class or walk_files function is responsible for locating the correct audio files. This makes it impossible to reason about the behavior of Dataset.

[vincentqb]

I'm ok with your suggestion @mthrok. I updated the description to reflect it. I'll leave this issue open until all the datasets use the same structure :)

[mthrok]

An approach better than walk_files is to delegate the responsibility of finding directories where audio files are found to Dataset, and the responsibility to traverse audio files in the directories in fixed order to a utility function. That way, it is clear who is responsible for what.

[vincentqb]

An approach better than walk_files is to delegate the responsibility of finding directories where audio files are found to Dataset, and the responsibility to traverse audio files in the directories in fixed order to a utility function. That way, it is clear who is responsible for what.

This is similar to having a deterministic walk_files to detect audio files and a filter to preserve only files matching a certain pattern, no?

[mthrok]

This is similar to having a deterministic walk_files to detect audio files and a filter to preserve only files matching a certain pattern, no?

No it is not. That approach does not delegate the responsibility of designating the directories to be looked at. That way no one is responsible at finding the correct directories that each Dataset implementation expects.

[vincentqb]

That way no one is responsible at finding the correct directories that each Dataset implementation expects.

Ah, I see what you mean now: the objection is that os_walk is currently used to detect the folders containing the audio files, and would also recurse sub-folders with more audio files.

Let's clarify what we want from our datasets:

1. The first installation/download of dataset produces a standard representation of the dataset.
2. The order of data points is deterministic for all users.
3. A signal/error is produced when information for a data point is missing, beyond the original form of the dataset.
4. New/old data points can be added/removed in a documented way.
5. A Dataset dataset can be "easily modified" to an IterableDataset, without storing a list representing the dataset (see also torchtext release notes)
6. Datasets have similar implementations and a new dataset is "short/easy" to implement by porting the code of an existing dataset.
7. [Compatibility with collate functions]

where a data point corresponds to an audio file, utterance, etc. I see the original issue about re-alignment to be about 6, and this discussion about deleguation to be about 4. Thoughts?

[mthrok]

4. New/old data points can be added/removed in a documented way.

I am against adding this as requirement. This makes the requirements for torchaudio's library Dataset implementation too vague and open, without us not knowing if it is really valuable. Also if the Dataset implementation is easy to port (by 6), then why does this have to be a requirements of the library Dataset?

6. Datasets have similar implementations and a new dataset is "short/easy" to implement by porting the code of an existing dataset.

I do not think Datasets have similar implementations should be a requirement and I do not see the value of it as a requirement. Rather it should be the outcome of the later part of short implementation. To achieve this, the design of utility function and delegation of responsibility is very important, which walk_files does not qualify to be applied.

I think these requirements are adding too much complexity and making it vague what Dataset implementation should do in the first place. Rather than starting from trying to incorporate all of them, we should keep it simple and stupid, then add other features incrementally.

[vincentqb]

Thanks for the feedback :)

4. New/old data points can be added/removed in a documented way.

I am against adding this as requirement. This makes the requirements for torchaudio's library Dataset implementation too vague and open, without us not knowing if it is really valuable. Also if the Dataset implementation is easy to port (by 6), then why does this have to be a requirements of the library Dataset?

That's fair objection. What I understand from what you are saying is that the goal of replacing os_walk isn't 4 above. Is that correct? I'm ok not having this as a requirement.

Just to clarify the comment above: are you saying instead that we should return errors as soon as the dataset structure differs from the intended application? say someone removes a whole data point (a file with all association information) ?

1. Datasets have similar implementations and a new dataset is "short/easy" to implement by porting the code of an existing dataset.

I do not think Datasets have similar implementations should be a requirement and I do not see the value of it as a requirement. Rather it should be the outcome of the later part of short implementation. To achieve this, the design of utility function and delegation of responsibility is very important, which walk_files does not qualify to be applied.

That's reasonable to me too and I have no problem replacing walk_files: I'm just hoping to capture and document why we want to replace so that we can maintain this into the future. :)

I think these requirements are adding too much complexity and making it vague what Dataset implementation should do in the first place. Rather than starting from trying to incorporate all of them, we should keep it simple and stupid, then add other features incrementally.

I definitely agree with you on keeping this very simple. SPEECHCOMMANDS is essentially two lines: os_walk and filter :) The advantage I see in leaving the folder navigation to the dataset is to capture modifications more quickly. I see this as "A signal/error is produced when information for a data point is missing." or maybe "A signal/error is produced when the dataset is modified."

I want to make sure we are on the same page. How about this until we know better? :)

1. The order of data points is deterministic for all users.
2. A signal/error is produced when information for a data point is missing.
3. New datasets can be ported from existing datasets in short implementations.
4. [A dataset shares implementation with an IterableDataset (see also torchtext release notes)]
5. [A dataset works with standard data loader collate functions.]

We don't need to aim for all of this immediately.

[vincentqb]

Superseded by #910.