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

[question] All checkpoints use the same VecNormalize statistics #278

Open
cameron-chen opened this issue Aug 16, 2022 · 4 comments
Open
Labels
bug Something isn't working enhancement New feature or request question Further information is requested

Comments

@cameron-chen
Copy link

Hi,

I find parameter save_freq and eval_freq enable us to save checkpoints during training:

  • save_freq: this saves checkpoints rl_model_{num_timesteps}_steps.zip
  • eval_freq: this saves checkpoint best_model.zip

However, we do not have normalization statistics at the moment we save them. There is only one vecnormalize.pkl saved by function save_trained_model() after training where we save checkpoint {env_id}.zip (refer to this link).

When we evaluate one of the checkpoints (rl_model_{num_timesteps}_steps.zip, best_model.zip, {env_id}.zip), we use the same normalization statistics (refer to this link). Does this affect the evaluation? Why do we not save the normalization statistics for every checkpoints?

Thank you!

@araffin araffin added bug Something isn't working question Further information is requested labels Aug 16, 2022
@araffin
Copy link
Member

araffin commented Aug 16, 2022

Hello,
good point, i did that mainly to save space, but you are right, we should give the ability to save each checkpoint stats too.

The other thing is that the normalization should converge at some point and therefore using the last stats for evaluating earlier checkpoints should not affect too much the results.

@cameron-chen
Copy link
Author

cameron-chen commented Aug 17, 2022

It makes a lot of sense to me. Thank you for reply.

Saving each checkpoint stats may cost space and, in general, is not necessary. Probably, It is reasonable and practical to save the stats of best_model.zip and {env_id}.zip.

p.s. the code does save checkpoint stat for best_model.zip (refer to this link) but finally the stats is replaced by the checkpoint stats for {env_id}.zip.

@araffin
Copy link
Member

araffin commented Sep 21, 2022

Saving each checkpoint stats may cost space and, in general, is not necessary. Probably, It is reasonable and practical to save the stats of best_model.zip and {env_id}.zip.

I would welcome a PR that does this ;)

@cameron-chen
Copy link
Author

cameron-chen commented Nov 4, 2022

Hi, I actually implemented one for personal use. Let me clean it and make a PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request question Further information is requested
2 participants