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

Log warning instead of fatal when parsing float get under/overflow. #4336

Merged
merged 3 commits into from
Jun 18, 2021

Conversation

cyfdecyf
Copy link
Contributor

@cyfdecyf cyfdecyf commented Jun 3, 2021

For texts that resolve to infinity, under or overflow should be accepted.

This should resolve unit test failure mention in #4330.

For texts that resolve to infinity, under or overflow should be
accepted.
@StrikerRUS
Copy link
Collaborator

Please update cpp tests code.
https://github.com/microsoft/LightGBM/blob/master/tests/cpp_tests/test_common.cpp
Right now UnderOverFlow test is failing with:

...

[ RUN      ] AtofPreciseTest.UnderOverFlow
[LightGBM] [Warning] convert to double got underflow or overflow: 1e+400
/Users/runner/work/1/s/tests/cpp_tests/test_common.cpp:108: Failure
Expected: LightGBM::Common::AtofPrecise("1e+400", &got) throws an exception of type std::runtime_error.
  Actual: it throws nothing.

...

@cyfdecyf
Copy link
Contributor Author

@StrikerRUS Thanks for spotting this. I ran a single test case and didn't notice there's outdated test case.

Fix pushed.

Copy link
Collaborator

@StrikerRUS StrikerRUS left a comment

Choose a reason for hiding this comment

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

Thank you!

I don't think I'm qualified to actually approve this PR, so just commenting.

Copy link
Collaborator

@shiyu1994 shiyu1994 left a comment

Choose a reason for hiding this comment

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

@cyfdecyf Thanks for your contribution. The changes LGTM.

@jameslamb jameslamb added fix and removed maintenance labels Jun 18, 2021
@jameslamb jameslamb merged commit f126db6 into microsoft:master Jun 18, 2021
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants