CNN Support Added #94
Conversation
|
@Prasanna28Devadiga Thanks for the PR. This looks better than the last one. I will review it in detail asap. I may add some comments if I found something that can be optimized or improved |
| We stick with the defaults in case the user doesnt provide them | ||
|
|
||
| """ | ||
| with open(path, 'rb') as f: |
There was a problem hiding this comment.
PS: You can use the read_yaml function from utils.py to read a yaml file
| """ | ||
| with open(path, 'rb') as f: | ||
| conf = yaml.safe_load(f.read()) | ||
| self.batch_size= conf['model']['arguments'].get('batch_size',32) |
There was a problem hiding this comment.
You can move all default values to a defaults.py or config.py file. This will make it easier to read and maintain the code
There was a problem hiding this comment.
Yes right i should be doing that
| pool_size_2d= tuple(pool_size_2d) | ||
| pool_size_3d= tuple(pool_size_3d) | ||
|
|
||
| if x == "Dense": |
There was a problem hiding this comment.
can we get rid of these if else? maybe with a dict/map and a function...
There was a problem hiding this comment.
Hey thats a pretty good idea. The challenge i was faced with was to map these strings to the respective class layers. I think i should be able to use a dictionary to do that that , and maybe just remove all this clutter from the main file
| class_mode: "sparse" | ||
| target_size: [28,28] | ||
| loss: "SparseCategoricalCrossentropy" | ||
| model_layers: |
There was a problem hiding this comment.
why not use the example I posted in the discussion comments? #76
There was a problem hiding this comment.
No particular reason actually. I'll make commits with the example you mentioned if thats required.
| #model.summary() | ||
| return model | ||
|
|
||
| def generate_dataset(self): |
There was a problem hiding this comment.
will this function work for all datasets? is it dynamic or it would only work for the mnist example
There was a problem hiding this comment.
Oh yes its dynamic. All it does is read the data from a data frame named as train.csv , and then create two image data-loaders.
the first column of the dataframe will be assumed to contain the paths , and the next column the corresponding labels.
Talking about mnist though i just realized the default value for the imagesize variable is (784,). This obviously is a bug as the default value should be (256^2, ) considering the data generator resizes all images to (256,256). I'll fix this with the next commit.
|
@nidhaloff Thank you for the detailed review. This was extremely insightful. As a result I've noticed a few key areas where improvements can be made , and some bug fixes as well. |
Solves issue #77
Model is configured via the YAML file. Have provided an yaml file example. Tested on Colab. Utilising Keras ensured that we could support a variety of layers besides just the standard Conv2D. Have added docstrings as per the contributing guidelines
@nidhaloff Is this adequate? Kindly give me a review