Closed AlbinSou closed 7 months ago
This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Files with Coverage Reduction | New Missed Lines | % | ||
---|---|---|---|---|
avalanche/training/supervised/lamaml_v2.py | 60 | 0.0% | ||
<!-- | Total: | 60 | --> |
Totals | |
---|---|
Change from base Build 7933962330: | 0.1% |
Covered Lines: | 20155 |
Relevant Lines: | 31410 |
Made a few clarification changes in IncrementalClassifier and Multiheadclassifier by replacing occurences of adaptation() implementation by train_adaptation and eval_adaptation, where if the eval_adaptation needs to be the same than train_adaptation I just call train_adaptation inside eval_adaptation (nested adaptation() override were a bit complex to understand and could lead to confusion)
I think we could also remove train_adaptation
and eval_adaptation
and have a single method. After, you can detect train/eval mode with a single if when necessary, so it seems to be adding unnecessary complexity in the common case.
@AntonioCarta How would you like to manage the adapt() and adaptation() function ? I wanted to keep the name of the "adaptation" also to keep backward compatibility but now it's a bit confusing to have two methods adapt() and adaptation()
@AntonioCarta How would you like to manage the adapt() and adaptation() function ? I wanted to keep the name of the "adaptation" also to keep backward compatibility but now it's a bit confusing to have two methods adapt() and adaptation()
The advantage of the current solution is that it's backward compatible. If we don't want to rename the old method we could rename the new one as rec_adapt
for recursive adaptation?
I was also thinking about a test case to check that the recursive calls don't break the #1591 fix:
model = ABasicDynamicModule(ABasicModule(MultiTaskClassifier(...)))
model.adapt(exp) # here it's calling rec_adapt from the "base" layer, not the MultiTaskClassifier. It should still work.
# assert that wrong heads are not adapted
@AntonioCarta How would you like to manage the adapt() and adaptation() function ? I wanted to keep the name of the "adaptation" also to keep backward compatibility but now it's a bit confusing to have two methods adapt() and adaptation()
The advantage of the current solution is that it's backward compatible. If we don't want to rename the old method we could rename the new one as
rec_adapt
for recursive adaptation?I was also thinking about a test case to check that the recursive calls don't break the #1591 fix:
model = ABasicDynamicModule(ABasicModule(MultiTaskClassifier(...))) model.adapt(exp) # here it's calling rec_adapt from the "base" layer, not the MultiTaskClassifier. It should still work. # assert that wrong heads are not adapted
I added a test for that at the end of test_models.DynamicModulesTest. Yes, maybe I will rename to make it clearer. Do you know how I can fix these failed Coveralls?
Made a few clarification changes in IncrementalClassifier and Multiheadclassifier by replacing occurences of adaptation() implementation by train_adaptation and eval_adaptation, where if the eval_adaptation needs to be the same than train_adaptation I just call train_adaptation inside eval_adaptation (nested adaptation() override were a bit complex to understand and could lead to confusion)
I think we could also remove
train_adaptation
andeval_adaptation
and have a single method. After, you can detect train/eval mode with a single if when necessary, so it seems to be adding unnecessary complexity in the common case.
Yes I agree, I don't remember why we included them in the first place
@AntonioCarta On my side everything is ok. I was thinking maybe to include an example on how to use dynamic modules outside and inside of a strategy (call to recursive_adaptation and explanation on how avalanche_model_adaptation works). Another thing is that I changed the location of avalanche_model_adaptation for now it's inside dynamic_modules.py before it was in models/utils.py. For now I import it in utils so that ppl don't have to change their imports, but it's a bit weird. The reason I put it inside dynamic_modules is that this function is needed in recursive_adaptation() method so putting it to utils was leading to annoying recursive imports.
I also just noticed that NCM is still using eval_adaptation so I will need to change that. I don't know how this has not been caught by some tests.
Ok. I will wait to merge this after release to be on the safe side.
I was thinking maybe to include an example on how to use dynamic modules outside and inside of a strategy
we already have examples of that in the notebooks. Maybe you can just update them?
I tried to implement the changes that would fix bugs described in Issue #1591 and also allow users to use Dynamic modules more easily outside of avalanche strategies.
To do so, I made several major changes.
For now tests do not pass, but some of them I am not sure what behaviour they were checking and if we really want this behaviour. I will also add some more tests to check that the behaviour in #1591 is solved (my local test says it is fixed for now).