Support negative values

[pgimalac]

Add support for negative values.
I wanted to use this package to print a human readable value for a size diff, but currently it's always printed in bytes, which is not very helpful.

Before:

>>> convert_units(-2**30)
(-1073741824, 'B')

After:

>>> convert_units(-2**30)
(-1.0, 'GiB')

[gemini-code-assist]

Hello @pgimalac, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

Summary of Changes

This pull request adds support for negative values to the convert_units function in binary/core.py. Previously, the function only handled positive values, always returning the absolute value in bytes when a negative value was passed. Now, the function correctly converts negative values to the appropriate unit (KiB, MiB, GiB, etc.) while preserving the negative sign. This enhancement is useful for scenarios like displaying size differences, where negative values indicate a reduction in size. The pull request also includes updated tests in tests/test_core.py to verify the correct handling of negative values.

Highlights

• Negative Value Support: The convert_units function now correctly handles negative input values, converting them to appropriate units while preserving the negative sign.
• Test Cases: Added new test cases to tests/test_core.py to ensure the correct behavior of convert_units with negative values, covering both binary and decimal units, as well as exact conversions.

Changelog

• binary/core.py
  • Modified the convert_units function to use abs(b) when comparing the input value to unit thresholds, allowing negative values to be correctly converted to appropriate units.
  • The conditional checks for unit conversion now use the absolute value of the input b to determine the appropriate unit prefix.
• tests/test_core.py
  • Added new test cases to verify the correct handling of negative values in the convert_units function.
  • Added assertions for negative values in TestConvert and TestConvertFloatExact classes, covering various units (B, KiB, MiB, GiB, TiB, PiB, EiB, ZiB, YiB, KB, MB, GB, TB, PB, EB, ZB, YB) and both binary and decimal prefixes.
  • Added assertions for negative values in TestConvertUnknownTo class.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

---

A byte's a byte,
Unless it's minus,
Then units shift,
With nary a gift,
Of positive sinus.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[gemini-code-assist]

Code Review

This pull request introduces support for negative values in the convert_units function, which is a valuable enhancement. The changes involve adding abs() calls to the conditional checks in binary/core.py and adding tests for negative values in tests/test_core.py. Overall, the changes seem correct and well-tested. However, I have a few suggestions for improvement.

Merge Readiness

The code changes seem correct and well-tested. The addition of abs() to the conditional checks in binary/core.py correctly handles negative values, and the added tests in tests/test_core.py provide good coverage. I don't see any critical or high severity issues, so this pull request is in good shape to be merged. However, I am unable to directly approve the pull request, and users should have others review and approve this code before merging.

[pgimalac]

I realize this fixes #10.

[ofek]

Please check if this allows you to remove the type checking stuff:

Suggested change

	babs = abs(b)
	babs: Union[float, Decimal] = abs(b)

[pgimalac]

That's what I tried initially but no, whatever my IDE uses for typing understands that it's a float or a Decimal, but mypy thinks it's an object, so it fails...

mypy: commands[0]> mypy .
binary/core.py:172: error: Incompatible types in assignment (expression has type "object", variable has type "float | Decimal")  [assignment]
        babs: Union[float, Decimal] = abs(b)
                                      ^~~~~~
Found 1 error in 1 file (checked 4 source files)

[ofek]

Weird, then I guess you'll have to do typing.cast(...)

[pgimalac]

I didn't know about that function ! Done, and the linter passes

[ofek]

/gemini would you recommend a more performant way to achieve this?

[gemini-code-assist]

That's a great question about performance, @ofek!

The current approach in the convert_units function uses a series of if/elif statements based on the absolute value of the input b (babs) to determine the appropriate unit. This is straightforward and easy to understand.

For the current number of units (up to Yobibyte/Yottabyte, which is 8 levels), the performance difference between this approach and a more complex one is likely negligible in most practical scenarios. The overhead of function calls and other operations might outweigh the cost of these comparisons.

However, if you were dealing with a significantly larger number of units or extremely performance-sensitive contexts, a theoretically more performant approach could involve using logarithms.

The idea would be to calculate the logarithm of babs with the appropriate base (1024 for binary, 1000 for SI). The integer part of the result would give you the index of the correct unit in a pre-defined list or dictionary of units and their corresponding divisors. This replaces the series of comparisons with a single logarithmic calculation and a lookup.

For example, for binary units, you could calculate index = int(math.log(babs, 1024)). You would then need to handle the base case (index 0 for Bytes) and use the index to retrieve the unit string and the divisor (base ** index).

Considerations for the logarithmic approach:

• Complexity with Decimal: Implementing this while maintaining the exactness provided by Decimal for exact=True might require careful handling, potentially using decimal.Decimal.log10 and adjusting the base, or finding a Decimal-compatible logarithm function for arbitrary bases.
• Readability: The current if/elif structure is arguably more readable for this specific problem domain than a logarithmic calculation and lookup.
• Actual Performance Gain: For only 8 levels, the real-world performance improvement might be minimal and not justify the increased code complexity.

In summary, while a logarithmic approach is a valid technique for optimizing unit conversions with many levels, the current if/elif structure is likely sufficient and more maintainable for this library's scope. The performance gain from a logarithmic approach might not be significant enough to warrant the added complexity, especially considering the need to handle Decimal values accurately.

Ultimately, the best approach depends on the specific performance requirements and how many more units might be added in the future. For now, the current implementation seems like a good balance of correctness, readability, and performance.