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

fix(ignite): making state saveable, param_groups modifieable #1235

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

sebffischer
Copy link
Collaborator

It was impossible to save the state because of:

  • Scalar Torch Tensors cannot be loaded after serialization #1233
  • undefined tensors were part of the state such as max_exp_avg_sq for adam with amsgrad = FALSE. We now keep them as 0-sized tensors as undefined tensors are not serializeable. (The reason we keep them at all is that it simplifies the saving and loading of state dicts easier)

This PR also improves the tests by removing an unnecessary call to torch_manual_seed() that made the tests deterministic

fix(ignite): making state saveable, param_groups modifieable

It was impossible to save the state because of:

* mlverse#1233
* undefined tensors were part of the state such as
  max_exp_avg_sq for adam with amsgrad = FALSE.
  We now keep them as 0-sized tensors as undefined tensors
  are not serializeable.
  (The reason we keep them at all is that it simplifies the
   saving and loading of state dicts easier)

This PR also improves the tests by removing an unnecessary
call to `torch_manual_seed()` that made the tests deterministic
@sebffischer
Copy link
Collaborator Author

@dfalbel Can you tag this with 'lantern' again please? I forgot to test the saving of the state dict which was not possible.

@dfalbel dfalbel added the lantern Use this label if your PR affects lantern so it's built in the CI label Jan 15, 2025
@dfalbel
Copy link
Member

dfalbel commented Jan 15, 2025

Done! @sebffischer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lantern Use this label if your PR affects lantern so it's built in the CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants