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

Clarify why PytorchModel and CnnResampleLoss subclasses exist #443

Closed
louisfh opened this issue Jan 31, 2022 · 1 comment · Fixed by #466
Closed

Clarify why PytorchModel and CnnResampleLoss subclasses exist #443

louisfh opened this issue Jan 31, 2022 · 1 comment · Fixed by #466
Assignees
Labels
docs Related to documentation or tutorials but not src code

Comments

@louisfh
Copy link
Member

louisfh commented Jan 31, 2022

I think we should clarify why the subclasses of PytorchModel and CnnResampleLoss (e.g. Resnet18Multiclass, InceptionV3 class) exist. As I understand this is for backwards compatibility? We could explain why these model classes are implemented in the docstrings/or somewhere else in the docs, and mention that Resnet18Multiclass(classes) is equivalent to cnn.PytorchModel('resnet18',classes)...

@louisfh louisfh added the docs Related to documentation or tutorials but not src code label Jan 31, 2022
@sammlapp sammlapp added the 0.6.2 label Mar 1, 2022
@sammlapp sammlapp self-assigned this Mar 1, 2022
@sammlapp
Copy link
Collaborator

@louisfh there is one difference between Resnet18Multiclass(classes) and PytorchModel('resnet18',classes):

  • the former allows you to use separate training parameters (eg, learning rate) for the feature and classifier blocks
    Do you think we should mark the Resnet18Multiclass and Resnet18Binary classes as deprecated? Then perhaps even remove them in 0.7.0?
    If this is more of a niche use, we could remove it from the codebase but put an example in a notebook incase we/someone else would like use that functionality. Alternitavely, we could leave it in - I just don't want to needlessly confuse the average user.

On second thought, Since the Resnet18Binary and Resnet18Multiclass models now only appear in tutorials during "Advanced CNN training" specifically for the feature they are meant for, I think we can probably leave them without deprecating/removing. Avg user will never know they exist.

@sammlapp sammlapp added the resolved_in_branch The issue has been resolved in a feat/issue branch but not merged into develop label Mar 21, 2022
sammlapp added a commit that referenced this issue Mar 21, 2022
clarifies differences between Resnet18Multiclass/Resnet18Binary and 
pytorchmodel in the class docstrings
sammlapp added a commit that referenced this issue Mar 25, 2022
@sammlapp sammlapp linked a pull request Mar 25, 2022 that will close this issue
@sammlapp sammlapp added resolved_in_develop The issue has been resolved in the develop branch but not in master and removed resolved_in_branch The issue has been resolved in a feat/issue branch but not merged into develop resolved_in_develop The issue has been resolved in the develop branch but not in master labels Mar 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Related to documentation or tutorials but not src code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants