-
Notifications
You must be signed in to change notification settings - Fork 2
Init weights #46
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
Init weights #46
Conversation
GwenaelleSa
left a comment
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.
A few WIP comments left.
The new parameter in the config files should be described in the readme of the repo.
cleherny
left a comment
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.
Do you want me to make the change here? Or do you?
Anyway, I'd like to modify the post-processing script for the quarry example to make it consistent with the proj-dqry. So I can make the change directly in this branch if it's easier.
acerioni
left a comment
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.
I've suggested an updated. If accepted, edits to the configuration files can be discarded.
scripts/train_model.py
Outdated
| MODEL_ZOO_CHECKPOINT_URL = cfg['model_weights']['model_zoo_checkpoint_url'] | ||
| else: | ||
| MODEL_ZOO_CHECKPOINT_URL = None | ||
| INIT_MODEL_WEIGHTS = cfg['model_weights']['init_model_weights'] |
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.
I suggest to update as follows:
INIT_MODEL_WEIGHTS = cfg['model_weights'].get('init_model_weights', False)
which would set INIT_MODEL_WEIGHTS to False by default, should the parameter not be provided by the config file. This would allow us to avoid updating all the existing configuration files.
This comment has been minimized.
This comment has been minimized.
GwenaelleSa
left a comment
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.
It was getting messy, so I added to the mess by doing the modifications I asked for and approving the PR.
@cleherny Thank you for your proposition, but I think we can merge this PR much faster than the bigger one on the DQRY example, so it's worth to keep this separated.
@acerioni You comment brought a good idea. I implemented it the same way we did for other optional arguments of the OD, meaning with a if-else statement rather than the get method. The result should be the same if I understood you correctly.
I don't merge because it's not very catholic knowing I was the last to modify this branch, but if @acerioni or @cleherny you agree with the modification, feel free to do it.
|
Thanks @GwenaelleSa for your latest edits 👍 |
No description provided.