Unified, centralized and simplified code; added CPU option#6
Unified, centralized and simplified code; added CPU option#6mbuet2ner wants to merge 6 commits intofg91:masterfrom
Conversation
better maintainability
better maintainability
notebooks do not have their own implementation of the code but rely on the centralized files
|
Check out this pull request on You'll be able to see Jupyter notebook diff and discuss changes. Powered by ReviewNB. |
|
@fg91 sorry for the mess. I know it is hard to review, because it combines quite a lot of changes in one PR. Should I make several pull requests out of it? |
| opt_steps_ = int(opt_steps * 1.3) | ||
| else: | ||
| opt_steps_ = opt_steps | ||
| train_tfms, val_tfms = tfms_from_model(self.model, sz) |
There was a problem hiding this comment.
This is done in the get_transformed and most_activated methods as well. How about making these instance attributes?
There was a problem hiding this comment.
Oh you are right. I overlooked that I'm afraid. I will change that!
|
Hey, Would you be willing to change the following?
Thanks! Fabio |
| activations = SaveFeatures(layer) # register hook | ||
| self.model(V(transformed)[None]) | ||
|
|
||
| print(activations.features[0, 5].mean().data.cpu().numpy()) |
There was a problem hiding this comment.
Why print especially activations.features[0, 5]? This was from the experiments folder, correct? :)
There was a problem hiding this comment.
Yep. Sorry for that. I will also change that!
There was a problem hiding this comment.
No need to apologize, I'm happy you put effort into improving the code, thanks for that!
There was a problem hiding this comment.
The
experimentsfolder contains additional notebooks provided by a reader of the blog post, so I would like to keep theFilterVisualizerseparated from it. Could you please move thefiltervisualizer.pyout of theexperimentsfolder? Theplot_reconstructions.pycan stay in the experiments folder as it is only used for the additional experiments...
I will move it to the root then!
Would you be willing to simplify the
Calculate_mean_activation_per_filter_in_specific_layer_given_an_image.ipynbusing themost_activated method? fromFiterVisualizer`?
Yes! I was thinking thinking about it already but decided not to, because
- the notebook would be basically empty then (perhaps I can combine the two then?) and
- I was not sure whether to include the
thresholdin the method or not.
IMHO it makes more sense to do thethresholdoutside of the method.
What do you think?
There was a problem hiding this comment.
- Yes, I agree to not include the threshold in the method.
- I would prefer to keep two separate notebooks even though there is not that much in the second one because I would have to go through the blog post and remove any hyperlinks to the 2nd notebook ahah
There was a problem hiding this comment.
Okay, that does make sense! I will apply the discussed changes and keep both notebooks separated then! 😃
adds parent dir to sys.path
train_tfms and val_tfms
prior to that it used its own implementation
|
I finally addressed your proposed changes 🎉 :
|
|
@fg91 If there is anything else you want me to change, just let me know. |
2 participants
Here are some things I added/ changed:
filtervisualizer.pyfileplot_reconstructions.pyfilefilterfunctionThings I have not changed yet, but will do in the future:
Thus, it should be much easier to change things in the future/ migrate to new fastai version.