-
Notifications
You must be signed in to change notification settings - Fork 75
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
[ minor ] bugfix in printing error log (model_commo_properties / LossScale) #2825
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but in this case, how about comparing float with stricter standard?
something like:
if (std::fpclassify(value) == FP_ZERO)
...
7fbf3ca
to
a421dc2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check the return.
@@ -43,7 +43,8 @@ ModelTensorDataType::ModelTensorDataType(ModelTensorDataTypeInfo::Enum value) { | |||
LossScale::LossScale(float value) { set(value); } | |||
|
|||
bool LossScale::isValid(const float &value) const { | |||
ml_loge("Loss scale cannot be 0"); | |||
if (std::fpclassify(value) == FP_ZERO) | |||
ml_loge("Loss scale cannot be 0"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's FP_ZERO, return a proper value.
The current return (value != 0
) is inappropriate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed it! Thank you for pointing it out. I updated the part. Thank you.
- This commit fixes a minor error to print error log. - This commit adds a condition to print the error log. Signed-off-by: Eunju Yang <[email protected]>
a421dc2
to
e55ac2c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Relevant issue: #2826
Self evaluation: