Generative Replay #931
Generative Replay #931
Conversation
into init of solver strategy.
extend GR plugin to work without generator initialization; clean up GR template and make it more modular; rename VAETraining
VAETraining can now be trained alone, with or without GR simply by adding the Plugin.
Modularization.
|
Hey @travela thanks for your contribution. It looks solid in general, I just left a couple of minor comments. |
Pull Request Test Coverage Report for Build 1958289232
💛 - Coveralls |
Hi @AntonioCarta, great thanks! I don't seem to be able to see your comments when looking in the "Files changed" tab. Did you publish them or could it be that they are still pending? And yes, I reproduced the results on the 10 classes splitMNIST scenario in |
| # Sample data from generator | ||
| memory = self.generator.generate( | ||
| len(strategy.adapted_dataset) * | ||
| (strategy.experience.current_experience)).to(strategy.device) |
There was a problem hiding this comment.
can you explain why the generated data has the same length of the original data? Can't we generate the data on-demand with a dataloader?
There was a problem hiding this comment.
Good question! There are two approaches (as mentioned here): either we generate all data needed for the experience beforehand and then start the training, or we save a copy of the "old" generator and generate our data on demand. I chose the former approach also because this way I could stay in line with the existing Replay strategy, where we update the strategy.data_loader once before each experience.
Do you you think the other method would be more efficient? How would the implementation of an additional dataloader roughly look like? Since the memory variable is only used temporarily and it is then passed to the ReplayDataloader in the next step, I was hoping that we are indeed already making use of the dataloader properties and do not clog RAM too much (but I am not entirely sure about this).
There was a problem hiding this comment.
Both are reasonable solutions. If you want to use this one, you should:
- batch the results of the Generator. In general you want minibatch size < data size, because the entire dataset may be too large to generate in one step.
- for the same reasons, unless the dataset is small, it's better to keep it in cpu and move only the minibatch to the gpu when needed.
If you don't do this you will get an out-of-memory error.
There was a problem hiding this comment.
I see. How would the first point tie in with the ReplayDataLoader? I could generate the data one mini-batch at a time to make it more resistant for larger datasets, but then I would still have to obtain an AvalancheDataset (ie. to concatenate them again?). This is because ReplayDataLoader expects an AvalancheDataset and not some kind of Dataloader, if I understand it correctly?
There was a problem hiding this comment.
This is because ReplayDataLoader expects an AvalancheDataset and not some kind of Dataloader, if I understand it correctly?
Exactly. I would make another dataloader similar to ReplayDataLoader that accepts a dataset (the current data), and an iterator (data generator).
There was a problem hiding this comment.
I definitely like the idea of using a Python Generator. But that would then correspond to the second of the two approaches I mentioned above, as when yielding the next batch we need access to the old version of the generator. I was trying to avoid having to store an additional model as well as trying to stick to the existing ReplayDataLoader as I understood it to be designed for any "rehearsal/replay strategies".
I am aiming to update the pull request by the beginning of next week.
There was a problem hiding this comment.
Hi! I just merged my changes (bba78b9). As mentioned in my last comment I now opted for the "on-demand" replay data generation, where we store the old generator+model and create replay data before each training iteration. This way I got around using the whole ReplayDataLoader syntax and instead just extend the current mini-batch with newly generated replay data.
Accuracy on splitMNIST stayed the same.
There was a problem hiding this comment.
I saw your implementation and IMO is clearer now.
Only a note:
when you create new data, you create as much replay data as the current minibatch dimension times the experience counter. This can result in huge minibatches (especially in benchmarks with a lot of experiences) that can easily fill the GPU memory if cuda is used, especially with datasets with bigger images than MNIST.
We should take this into consideration if we want to use this implementation as the starting point for all the generative replay strategies.
There was a problem hiding this comment.
Good point. I have actually implemented an alternative solution where we use a weighted loss to gradually reduce the importance of a new experience/class as the total number grows. As mentioned below, this solution yielded a lower accuracy (in my particular example). However not using any of the two methods yielded much lower accuracy, suggesting that at least one of the two is necessary.
When implementing the weighted loss I had to overwrite the criterions of both, the model and the generator, which maybe makes it less intuitive, but I guess proper documentation will take care of that.
I was thinking of adding the weighted loss as an option for the user. Maybe I could make it the default option and offer the increasing minibatch size as an alternative (for simple cases like MNIST)?
There was a problem hiding this comment.
Makes sense. Consider that the simple replay in Avalanche does not change the batch size, so it would be better to have consistent behavior here.
* Extend current mbatch with replay data dynamically before each iteration. * Update boolean after first experience. * Fix mbatch[-1] extension * Put replay_output to device. * Resolve change requests: class names; VAELoss doc * Documentation.
There was a problem hiding this comment.
Very nice work!
I added some comments and little requests, but this seems a great starting point!
I labeled this review as request changes only to follow in details the next steps, but the work to be done is fairly low.
As a general comment, many classes are named quite generally (Generator, VAE, VAEEncoder..) but lack this generality in the implementation (e.g. a CNN VAE). I'd suggest to call them in different ways (e.g. SimpleVAE or MlpVAE) in order to don't have the need, in the future, to rename them losing compatibility with old code.
| or we use the strategy's model as the generator. | ||
| If the generator is None after initialization | ||
| we assume that strategy.model is the generator.""" | ||
| if not self.generator_strategy: |
There was a problem hiding this comment.
This is clear but confusing. If I understand well the plugin needs a generative strategy, which contains the generative model. If no strategy is passed, the "default" strategy is used, and the model defined for the strategy is used as generative model. In this case, is the generative model the only model used? (no classifier).
There was a problem hiding this comment.
Yes exactly: if no generative strategy is passed, we assume that the strategy our plugin was added to already has a model which is generative. E.g. this allows us to easily train a generator with generative replay by simply adding the plugin. In that case there would be no classifier. (Another scenario is that the classifier and generator are combined in a single model, as in this paper about Generative replay with feedback connections).
I agree that it can be confusing (a result of trying to combine all scenarios in a single plugin). I added another line in the doc string (4f5246f) to refer to the example of training a generator, hoping this would make it clearer. Or do you have another suggestion?
There was a problem hiding this comment.
No it's fine, I read the code sequentially, so I didn't have a complete idea until the end.
I still believe is a bit confusing, but I don't have a better idea at the moment.
I really like the generality of the strategy, and overall the pros of this implementation greatly outshine a bit of confusion in this part 😉
| replay = self.old_generator.generate( | ||
| len(strategy.mbatch[0]) * (strategy.experience.current_experience) | ||
| ).to(strategy.device) |
There was a problem hiding this comment.
Why for each minibatch the number of generated data increases with the experiences? Is it a particular implementation used in some paper?
There was a problem hiding this comment.
I actually got inspired to do this from this intro_to_generative_replay notebook from the Avalanche/colab repo. In the literature I usually see authors employ a weighted loss instead to achieve a similar effect (e.g. as described here in equation 3). It could be due to the VAE and classifier I use, but in my case the increasing generated data fared a few percentage points higher in accuracy than the weighted loss approach.
| def __init__( | ||
| self, | ||
| model: Module, | ||
| optimizer: Optimizer, | ||
| criterion=CrossEntropyLoss(), | ||
| train_mb_size: int = 1, | ||
| train_epochs: int = 1, | ||
| eval_mb_size: int = None, | ||
| device=None, | ||
| plugins: Optional[List[SupervisedPlugin]] = None, | ||
| evaluator: EvaluationPlugin = default_evaluator, | ||
| eval_every=-1, | ||
| generator_strategy: BaseTemplate = None, | ||
| **base_kwargs | ||
| ): |
There was a problem hiding this comment.
Really like the idea of having 2 different strategies, one for the classifier (this one) and one for the generative model (passed as an argument). This allows a great generalization.
Requested changes
|
Ciao @ggraffieti! Thanks a lot for the detailed review and the helpful suggestions. I added all requested changes just now. |
Optional enhancements
… generative_replay
|
@ggraffieti are there any other changes that you want on this PR? |
|
I checked the test manually and it seems to work. I'm merging this, and we can investigate the CI problems separately. @travela thanks for your contribution and please remember to push the reproducibility script to the reproducibility-repo. |
closes #927
This is adds a
GenerativeReplayPluginand aGenerativeReplaystrategy to the library with which one can train models according to the vanilla Generative Replay algorithm. One can either apply it to a generator model alone or a pair consisting of a classifier and a generator.In particular I have added two usage examples where we train two models on the splitMNIST scenario, i.e.:
examples/generative_replay_splitMNIST.pyandexamples/generative_replay_MNIST_generator.py