-
Notifications
You must be signed in to change notification settings - Fork 76
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Train add #20
Train add #20
Conversation
… checkpoint should now work. From scratch not implemented yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall, thanks @StephenHausler.
I have a few minor requests (see inline comments) as well as two bigger ones:
-
It would be great to move the
train
,test
andget_clusters
methods to separate files living inpatchnetvlad/training_tools
. Similarly, movesave_checkpoint
andhumanbytes
intopatchnetvlad/training_tools/tools.py
. -
The readme needs extending with describing how the training can be performed.
An added:
It would be nice to add train to
Line 62 in 0e44e34
'patchnetvlad-download-models=download_models:download_all_models'], |
Fix #8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me now, thanks! One very minor comment below (probably left from testing) We just need the PCA functionality and this is ready to go in my opinion.
@oravus - would be great if you could have a look, too.
All review comments completed, code has been tested and should be ready for merge. |
Thanks a lot @StephenHausler - let's merge it in :) |
No description provided.