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 the old_sigma assertion when lr_adapt=True #174

Merged
merged 3 commits into from
Apr 2, 2024

Conversation

Kreyparion
Copy link
Contributor

The current behavior : the assertion fails when lr_adapt=True and sigma is an integer (like sigma=1) which forces to put sigma = 1.0

With assert isinstance(old_sigma, (int, float)) we allow integer values for sigma

@nomuramasahir0
Copy link
Collaborator

Hi @Kreyparion ,

Thank you for the PR.
That's correct, and more than that, this assertion is not needed (as old_sigma is surely int or float).
Could you remove the assert instead of new assert?

@Kreyparion
Copy link
Contributor Author

Yes,
Thank you for pointing it out,
sigma has already been checked in __init__, so getting rid of the assertion shouldn't be a problem

The other three assertions are also always verified, as their types are always checked beforehand with the "current" variables.

But it might be a good idea to keep them for testing purposes.
Should I delete them too ?

@nomuramasahir0
Copy link
Collaborator

Yes, as you said, I think the other three assertions can be removed in the same reason.
I'm glad if you delete them.

@nomuramasahir0
Copy link
Collaborator

Thanks! Failed checks are not due to this PR, so let me merge it.

@nomuramasahir0 nomuramasahir0 merged commit bbeb640 into CyberAgentAILab:main Apr 2, 2024
11 of 13 checks passed
@nomuramasahir0
Copy link
Collaborator

#175
I'm sorry, I forgot that these assertions are needed for mypy; therefore, I eventually adopted your first suggestion.
Thanks again!

@Kreyparion Kreyparion deleted the fix_old_sigma_check branch April 30, 2024 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants