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

Use hydra for instantiating models and optimizers #295

Merged
merged 4 commits into from
Mar 24, 2022

Conversation

remtav
Copy link
Collaborator

@remtav remtav commented Mar 22, 2022

fixes #246 and #293 (breaking our one issue / one PR rule, forgive me!)
add tests for optimizer instantiation in test_optimizers.py
adapt our unet models (models/unet.py) to expect same parameter names as smp models

add tests for optimizer instantiation in test_optimizers.py
adapt our unet models (models/unet.py) to expect same parameter names as smp models
# '\nThe NIR modality will be fed at first layer of model alongside other bands,'
# '\nthe implementation of concatenation point at an intermediary layer is only available'
# '\nfor the deeplabv3 model for now. \nMore will follow on demand.'
# )
Copy link
Collaborator Author

@remtav remtav Mar 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This warning is broken since we wouldn't use cfg.model.model_name anymore. It's an easy fix, but not sure this warning is necessary. Our smp models can read NIR and recycle other bands' weights. Maybe some documentation could be added to our deeplav3 dualhead and that would be enough. @CharlesAuthier ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeaah its ok with me

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok! I'll remove this in PR #297

raise logging.critical(ValueError(f'\nThe model name {model_name} in the config.yaml is not defined.'))

return model
return instantiate(net_params, in_channels=in_channels, classes=out_classes)
Copy link
Collaborator Author

@remtav remtav Mar 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some hydra magic to rule out all if/else statements. wouhou!

# '\nThe NIR modality will be fed at first layer of model alongside other bands,'
# '\nthe implementation of concatenation point at an intermediary layer is only available'
# '\nfor the deeplabv3 model for now. \nMore will follow on demand.'
# )
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeaah its ok with me

@remtav remtav merged commit a1e322d into NRCan:develop Mar 24, 2022
@remtav remtav deleted the 246-hydra-model-choice branch March 24, 2022 00:18
remtav added a commit to remtav/geo-deep-learning that referenced this pull request Jul 5, 2022
* fixes NRCan#293 NRCan#246
add tests for optimizer instantiation in test_optimizers.py
adapt our unet models (models/unet.py) to expect same parameter names as smp models

* minor typo fixes

* name model yamls as close as possible to upcoming naming convention

* fix model name
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Model choice: maximize Hydra's features
3 participants