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: Handle NaN values during weight conversion #168

Merged
merged 2 commits into from
Jan 9, 2024
Merged

fix: Handle NaN values during weight conversion #168

merged 2 commits into from
Jan 9, 2024

Conversation

tgaddair
Copy link
Contributor

@tgaddair tgaddair commented Jan 9, 2024

Fixes #81.

Solution proposed by @asingh9530 .

Copy link
Contributor

@arnavgarg1 arnavgarg1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! LGTM, thanks for putting this together @tgaddair! Is it also worth adding special error checking if the weights have Inf (I know this is super rare and really only can happen when training without regularization measures in place)?

@asingh9530
Copy link
Contributor

asingh9530 commented Jan 9, 2024

@tgaddair looks good but not sure if nesting conditions would be right choice as if for any reason we in future it requires to add more check it would start becoming cluttered.

@tgaddair
Copy link
Contributor Author

tgaddair commented Jan 9, 2024

@asingh9530 it's a good point. My reason for doing it this way was to save the computation cost of calling torch.any(torch.isnan)) in cases where we know the weights are not nan. Not sure how much of an impact this will have in practice. Happy to go with an alternative if you feel there's a better place for this check!

@asingh9530
Copy link
Contributor

@tgaddair how about using a validatorTensor method and it takes care of validating tensors and then we can add validation conditions much neatly there. Let me know I can add in this PR as adding internal methods like validation or convert makes a huge difference in maintaining

@tgaddair
Copy link
Contributor Author

tgaddair commented Jan 9, 2024

Hey @asingh9530, that sounds good! I can go ahead and merge this PR for now and then you can create a follow-up PR, if that works.

@tgaddair tgaddair merged commit 112aeec into main Jan 9, 2024
1 check failed
@tgaddair tgaddair deleted the nan-error branch January 9, 2024 18:42
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.

Surface more informative error when adapter has NaN weights
3 participants