Skip to content
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

fix/adapt argument name for pretrained model #44

Merged

Conversation

linakrisztian
Copy link
Contributor

@linakrisztian linakrisztian commented Jul 24, 2023

When using pretrained models within trainig I experienced the --weights_path was not implemented anymore. I guess the name was just changed, so this small change should solve it.

@pesekon2 pesekon2 added the bug Something isn't working label Jul 24, 2023
@pesekon2
Copy link
Collaborator

@linakrisztian You are right, it is surely a bug (here we can see that I do not fine-tune often enough as the bug is in the code for quite some time). However, if you don't mind, I would like to keep the parameter name still weights_path and instead got rid of the model_path parameter check (-> weights_path). The reasons are two:

  1. The *.h5 file, no information on the model structure (connections) is written, as far as I know; it contains just values for all weights.
  2. It could bring some confusion when used together with model_fn and output_path (is model_path the output one or is it the one I am loading? Or is that model_fn)? A counter-argument: The naming of the parameters is stupid in general and things like model_fn should be renamed.

Or do you think that model_path is more understandable for the parameter? In such case, I would have no problem accepting the change.

@linakrisztian
Copy link
Contributor Author

No you are right, I would agree on that. weights_path is more understandable. I changed it accordingly.

@pesekon2
Copy link
Collaborator

Great, thank you.

@pesekon2 pesekon2 merged commit e1f7462 into ctu-geoforall-lab-projects:master Jul 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants