maxpumperla / deep_learning_and_the_game_of_go

Code and other material for the book "Deep Learning and the Game of Go"
https://www.manning.com/books/deep-learning-and-the-game-of-go
953 stars 387 forks source link

small bug in processor? #75

Closed nutpen85 closed 3 years ago

nutpen85 commented 3 years ago

Hi, first of all thank you so much for writing this book. It's really great. I have a short question about the code in processor and processor_parallel. The part I'm talking about looks like this:

    chunk = 0  # Due to files with large content, split up after chunksize
    chunksize = 1024
    while features.shape[0] >= chunksize:  # <1>
        feature_file = feature_file_base % chunk
        label_file = label_file_base % chunk
        chunk += 1
        current_features, features = features[:chunksize], features[chunksize:]
        current_labels, labels = labels[:chunksize], labels[chunksize:]  # <2>
        np.save(feature_file, current_features)
        np.save(label_file, current_labels)  # <3>

I have the feeling, that the line starting with while should be more like: while features.shape[0] >= chunk * chunksize: Because otherwise the loop might not even be executed once. But I'm also a newbie to python programming. So, maybe the code is fine as it is. But then it would be great to hear, where I'm thinking wrongly. Thank you.

nutpen85 commented 3 years ago

Ah, I'm so sorry. Last night I suddenly understood, what I was thinking wrong. I had a problem, when trying out the code with only loading 2 games, when the while loop was not executed for neither file. And so not npy-files were created to extract features and labels from. But in general the code is fine as it is. I'll close the issue.

maxpumperla commented 3 years ago

@nutpen85 thanks for reporting this anyway, and I'm glad you like the book!

Can you think of a good way of extending the code so that we can capture your edge case? It seems like the current error message didn't help you right away.

nutpen85 commented 3 years ago

Thank you for your comment. You are right. The error I got was somewhere else (np.concatenate(feature_list, axis_0) in the consolidate_games() method) which took me some time to figure out, where the actual problem is located.

I was thinking a bit how to solve this. The problem only occurs, when there are less then 1024 features drawn from a zip file, so about 5 games. This only happens, when you set the number of games rather small (well below 100). If you draw enough games from at least one zip-file, the error will not occur. However, if you already have files with the right name.npy stored you will instead load this file, because no other file was created. So, many people will not notice when they try small numbers of games later, but may instead load a much larger set of games.

So, what could be solutions to this? One easy way is to replace while features.shape[0] >= chunksize: with while features.shape[0] > 0: However, here I can imagine a problem with the generator, since there may be files created with less features than the batch_size. But I did some tests and there was no problem so far.

Of course one could also only throw a warning like if features.shape[0] < chunksize: print('Warning: No games used from ' + data_file_name + ' due to too small amount of randomly chosen games, which resulted in less than ' + chunkszie + ' features.)

This would at least make it easier to find for other people running into this problem.

What do you think?

maxpumperla commented 3 years ago

@nutpen85 maybe it's easier to catch zip files with less than, say, 10 games and just skip that altogether. While this is not a particularly accurate solution (you might lose a few games), it's quite clear and fails early.

yeah, so the chunksize is there to only continue generating data if there's enough data left. don't have time to check this, but I have a hunch that stuff will fail down the line if we replace by 0 (batch size too small for training etc.)