Handle NaN by The-Debarghya · Pull Request #63 · python-humanize/humanize

The-Debarghya · 2022-10-05T16:12:03Z

Fixes #62 Handling math.nan in metric() method

Changes proposed in this pull request:

Check if the value parameter is NaN and return if it is so.
The rest logic remains same.

carterbox

IN addition to comment below, docstring needs to be updated to specify what happens with nonfinite numbers. I believe floats can also represent inf, so it would be great if you used math.isfinite() to handle both inf and nan.

src/humanize/number.py

codecov-commenter · 2022-10-05T22:43:03Z

Codecov Report

Merging #63 (9245532) into main (c35fa9c) will decrease coverage by 0.92%.
The diff coverage is 72.00%.

@@            Coverage Diff             @@
##             main      #63      +/-   ##
==========================================
- Coverage   99.29%   98.36%   -0.93%     
==========================================
  Files           9        9              
  Lines         710      735      +25     
==========================================
+ Hits          705      723      +18     
- Misses          5       12       +7

Flag	Coverage Δ
macos-latest	`96.73% <72.00%> (-0.88%)`	⬇️
ubuntu-latest	`96.73% <72.00%> (-0.88%)`	⬇️
windows-latest	`95.51% <72.00%> (-0.83%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/humanize/number.py	`93.16% <68.18%> (-3.96%)`	⬇️
tests/test_number.py	`100.00% <100.00%> (ø)`
tests/test_time.py	`100.00% <100.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

hugovk · 2022-10-06T12:44:02Z

Please could you also add tests?

The inputs and expected outputs can go in this list:

humanize/tests/test_number.py

Lines 199 to 229 in c35fa9c

    
               "test_args, expected", 
        
               [ 
        
                   ([0], "0.00"), 
        
                   ([1, "Hz"], "1.00 Hz"), 
        
                   ([1.0, "W"], "1.00 W"), 
        
                   ([3, "C"], "3.00 C"), 
        
                   ([3, "W", 5], "3.0000 W"), 
        
                   ([1.23456], "1.23"), 
        
                   ([12.3456], "12.3"), 
        
                   ([123.456], "123"), 
        
                   ([1234.56], "1.23 k"), 
        
                   ([12345, "", 6], "12.3450 k"), 
        
                   ([200_000], "200 k"), 
        
                   ([1e25, "m"], "10.0 Ym"), 
        
                   ([1e26, "m"], "100 Ym"), 
        
                   ([1e27, "A"], "1.00 x 10²⁷A"), 
        
                   ([1.234e28, "A"], "1.23 x 10²⁸A"), 
        
                   ([-1500, "V"], "-1.50 kV"), 
        
                   ([0.12], "120 m"), 
        
                   ([0.012], "12.0 m"), 
        
                   ([0.0012], "1.20 m"), 
        
                   ([0.00012], "120 μ"), 
        
                   ([1e-23], "10.0 y"), 
        
                   ([1e-24], "1.00 y"), 
        
                   ([1e-25], "1.00 x 10⁻²⁵"), 
        
                   ([1e-26], "1.00 x 10⁻²⁶"), 
        
                   ([1, "°"], "1.00°"), 
        
                   ([0.1, "°"], "100m°"), 
        
                   ([100], "100"), 
        
                   ([0.1], "100 m"), 
        
               ],

for more information, see https://pre-commit.ci

The-Debarghya · 2022-10-07T05:03:15Z

Is it ok now or should I add more tests?

hugovk · 2022-10-09T08:29:17Z

@carterbox @bersbersbers Thanks for your reviews, how's it look now?

bersbersbers · 2022-10-09T08:32:20Z

Looks good for metric. Might handle other functions here or in a separate PR, #62 (comment)

The-Debarghya · 2022-10-09T09:11:49Z

Thanks for your feedback!
Should I make a separate PR for other methods or push further commits in this PR?

hugovk · 2022-10-09T09:32:07Z

Should I make a separate PR for other methods or push further commits in this PR?

I'd prefer pushing to this PR, but either way is fine :)

for more information, see https://pre-commit.ci

The-Debarghya · 2022-10-09T13:28:35Z

I have just raised a ValueError for nan and handled the inf case with OverflowError is this logic ok or it needs some tweaking.
Also if I need to add tests please mention.

bersbersbers · 2022-10-10T18:04:43Z

When you raise a ValueError, and in case of ValueError, the output is nan and inf, right? That is different from what we have for metric().

My rule thumb is, if you wonder if a test is necessary, a test is necessary.

The-Debarghya · 2022-10-11T10:53:20Z

Okay then adding the check for nan and inf in other methods.

bersbersbers · 2022-10-11T12:46:36Z

I wonder if it might make sense to have a format_non_finite method somewhere, so that each formatting function can simply check for isfinite (covering NaN and +/-Inf) and fall back to format_non_finite, which in turns handles conversion of each possible value into one globally consistent string output.

for more information, see https://pre-commit.ci

bersbersbers · 2022-10-12T18:43:31Z

src/humanize/number.py

+        return "NaN"
+    if math.isinf(value) and value < 0:
+        return "-Inf"
+    elif math.isinf(value) and value > 0:


You may be interested in https://pylint.pycqa.org/en/latest/user_guide/messages/refactor/no-else-return.html :)

bersbersbers · 2022-10-12T18:43:44Z

src/humanize/number.py

+        return "-Inf"
+    elif math.isinf(value) and value > 0:
+        return "+Inf"
+    else:


carterbox · 2022-10-12T21:17:21Z

src/humanize/number.py

+def format_non_finite(value: float) -> str:
+    """Utility function to handle infinite and nan cases."""
+    if math.isnan(value):
+        return "NaN"
+    if math.isinf(value) and value < 0:
+        return "-Inf"
+    elif math.isinf(value) and value > 0:
+        return "+Inf"
+    return ""


This function seems overly complicated and not necessary to me. Why not just use the built-in string conversions? "nan", "-inf", and "inf"?

if not math.isfinite(float(value)): return str(float(value))

My argument for NaN was the capitalization in https://en.wikipedia.org/wiki/NaN, which I also prefer. But I don't have a strong opinion on that.

hugovk · 2022-10-12T21:15:10Z

src/humanize/number.py

 NumberOrString: TypeAlias = "float | str"


+def format_non_finite(value: float) -> str:


Let's have this as a "private" function:

Suggested change

def format_non_finite(value: float) -> str:

def _format_non_finite(value: float) -> str:

I'm making it private...will it be merged then?

Will merge when it's ready, thank you for all your work!

What more to add?

src/humanize/number.py

See https://pylint.pycqa.org/en/latest/user_guide/messages/refactor/no-else-return.html

hugovk · 2022-10-26T20:42:03Z

Thank you for the PR!

Bug-Fix: handle NaN

6d2dcf6

carterbox suggested changes Oct 5, 2022

View reviewed changes

src/humanize/number.py Outdated Show resolved Hide resolved

handle NaN and Inf

414fe73

bersbersbers reviewed Oct 5, 2022

View reviewed changes

src/humanize/number.py Outdated Show resolved Hide resolved

handle inf

69dcb60

hugovk added the changelog: Fixed For any bug fixes label Oct 5, 2022

-inf and +inf

b4dbe70

The-Debarghya and others added 2 commits October 6, 2022 19:11

Add tests for inf and nan

f701644

[pre-commit.ci] auto fixes from pre-commit.com hooks

c8b859b

for more information, see https://pre-commit.ci

The-Debarghya and others added 3 commits October 9, 2022 18:18

Update number.py

3c61efd

Update number.py

f43a915

[pre-commit.ci] auto fixes from pre-commit.com hooks

14cbcb1

for more information, see https://pre-commit.ci

The-Debarghya and others added 7 commits October 12, 2022 21:01

Merge remote-tracking branch 'origin/main' into bug-62-handle-nan

31ad331

Update number.py

c0a1106

Add docstring in number.py

05f6c0f

[pre-commit.ci] auto fixes from pre-commit.com hooks

9245532

for more information, see https://pre-commit.ci

Add tests

b46e0d5

[pre-commit.ci] auto fixes from pre-commit.com hooks

e543798

for more information, see https://pre-commit.ci

Update number.py

6b33558

bersbersbers reviewed Oct 12, 2022

View reviewed changes

src/humanize/number.py Outdated

return "-Inf"

elif math.isinf(value) and value > 0:

return "+Inf"

else:

Copy link

bersbersbers Oct 12, 2022

Choose a reason for hiding this comment

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

Same

Update number.py

e7f5c2b

carterbox reviewed Oct 12, 2022

View reviewed changes

hugovk reviewed Oct 12, 2022

View reviewed changes

The-Debarghya and others added 2 commits October 13, 2022 10:28

Update number.py

b72d21c

Simplify

e64ad9e

See https://pylint.pycqa.org/en/latest/user_guide/messages/refactor/no-else-return.html

hugovk approved these changes Oct 17, 2022

View reviewed changes

hugovk merged commit ba53432 into python-humanize:main Oct 26, 2022

hugovk changed the title ~~Bug-Fix: handle NaN~~ Handle NaN Dec 30, 2022

The-Debarghya deleted the bug-62-handle-nan branch July 24, 2023 05:21

		NumberOrString: TypeAlias = "float \| str"


		def format_non_finite(value: float) -> str:

	def format_non_finite(value: float) -> str:
	def _format_non_finite(value: float) -> str:

Uh oh!

Conversation

The-Debarghya commented Oct 5, 2022

Uh oh!

carterbox left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Oct 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

hugovk commented Oct 6, 2022

Uh oh!

The-Debarghya commented Oct 7, 2022

Uh oh!

hugovk commented Oct 9, 2022

Uh oh!

bersbersbers commented Oct 9, 2022

Uh oh!

The-Debarghya commented Oct 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hugovk commented Oct 9, 2022

Uh oh!

The-Debarghya commented Oct 9, 2022

Uh oh!

bersbersbers commented Oct 10, 2022

Uh oh!

The-Debarghya commented Oct 11, 2022

Uh oh!

bersbersbers commented Oct 11, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hugovk commented Oct 26, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-commenter commented Oct 5, 2022 •

edited

Loading

The-Debarghya commented Oct 9, 2022 •

edited

Loading