-
-
Notifications
You must be signed in to change notification settings - Fork 295
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
CIL Examplar Free components #1528
Conversation
avalanche/models/cosine_layer.py
Outdated
|
||
class CosineIncrementalClassifier(DynamicModule): | ||
# WARNING Maybe does not work with initial evaluation | ||
def __init__(self, in_features, num_classes): |
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.
num_classes should be optional in the incremental version? (1 is a good default)
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.
About that, I am not sure right now whether this behavior can be implemented with the current code state. In this code, whenever new classes arrive, the classifier switchs from using one CosineLinear layer to a SplitCosineLinear one (containing two CosineLinear). So, in the case we initialize with 1 class, this SplitCosineLinear will contain the old layer (with 1 logit) plus a new layer containing the new seen classes (let say 9 new ones for the first task). I think this behavior is a bit weird. But I agree that it would be nice to be consistent with the IncrementalClassifier behavior, I will make some more tests to make sure we can get a version that is both correct and consistent with the current IncrementalClassifier
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.
If I understand correctly, this means that you should not have a num_classes
parameter at all. You must always start with 0 units.
avalanche/models/cosine_layer.py
Outdated
def forward(self, x): | ||
return self.fc(x) | ||
|
||
def generate_fc(self, in_dim, out_dim): |
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.
internal method? use _generate_fc
. If it's public add doc.
Thanks @AlbinSou. Can we split this in multiple PRs? I think FeCAM and CosineLayer are ready once you add tests and doc. We can add the rest later. I'm not sure about the plugins. The decoupling between the modules and their logic makes it a bit more complex to use them but I don't have a solution for this. If there is no alternative solution, I would at least add a reference to their implementation in the Module's doc. |
I start this PR but it's not finished. Feel free to discuss the things that should be changed.
For now, here is what I added
Here is what I want to add: