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

Add division operator to close Issue #2 #9

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

johnMamish
Copy link

Hey!

This fixed point library has been extremely useful for a research project we are working on. We needed a division operator, so I decided to offer our implementation in case you are interested.

Precision of result

In the discussion under issue #2, Zack mentions that "fixed point division is ambiguous". To cast the broadest net, I implemented the highest precision version of division that is significant in fixed-point arithmetic (as described in Randy Yates's guide). From this starting point, users can truncate the result as desired.

Unit tests

I have neither a matlab license, matlab knowledge, or a windows computer, so the test I've written fits in very poorly with the rest of this codebase. I don't have the time right now to make a proper unit test, but if the maintainers are interested in this PR and would like me to do that, I'm glad to. Perhaps it would also be easy for the maintainers to write a unit test if they are interested in this PR.

Best

  • John

@zks0002
Copy link

zks0002 commented Jan 4, 2024

Hi John,

Thanks for your PR! I am no longer with Schweitzer Engineering Laboratories, so I cannot decide its fate, but I can weigh in here.

I didn't dig too deep into accuracy and correctness, but after looking through it and trying out a few things, the results make sense for the division operator. Kudos to you for using Randy Yates's document, though I must admit his "practical" rules may not cover all cases they folks wish to implement. E.g., 1/7 is an irrational number, so by definition there are infinite fractional bits. What he considers "practical" may be implemented completely different in custom digital logic, thus would require a different q format than what is "practical." Nonetheless, I think this is an excellent starting point!

A few points I want to bring up:

truediv == floordiv

The division operator result makes sense:

>>> from fixedpoint import FixedPoint
>>> a, b = FixedPoint(1), FixedPoint(7)
>>> float(a / b)
0.125
>>> float(a) / float(b)
0.14285714285714285

But you've set the division operator equal to the floor division operator, which gives:

>>> float(a // b)
0.125
>>> float(a) // float(b)
0.0

And that doesn't make mathematical sense to me. Floor division should return a whole number. Furthermore, according to Python 3's data model, "The __divmod__() method should be equivalent to using __floordiv__() and __mod__(); it should not be related to __truediv__()" and the examples above would not support that relationship.

floordiv, mod, and divmod

If you're going to implement a true floor division operator, it's not much more effort to do define valid __mod__ and __divmod__ (plus their relective and augmented) methods as well.

Unit Tests

The accuracy of the original unit tests were/are based on MATLAB's gold standard Fixed-Point libraries. MATLAB does not define a division operation for fixed point numbers, so the maintainers would need to specify a different way to accomplish unit testing. Your unit tests compare the accuracy of the fixed point result to a floating point value with a 52-bit mantissa. I suppose this is valid... but only for numbers with less than 52 (or whatever sys.float_info.mant_dig - 1 is) fractional bits 😆 But in your tests you have up to 200 bits in the randomly generated numbers, up to 100 fractional bits.

My gut feeling tells me that we should concoct some specific corner cases, as well as some random cases that will always yield an exact result. The hypothesis library might be perfect for this.

Conclusion

To be honest, I feel that __truediv__, __itruediv__, and __rtruediv__ should be left undefined because a single definition could not adequately cover all use cases. Nonetheless, this implementation could be referenced as an optional thing, e.g.,

from fixedpoint import FixedPoint
>>> x, y = FixedPoint(1), FixedPoint(2)
>>> x / y
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: unsupported operand type(s) for /: 'FixedPoint' and 'FixedPoint'
>>> FixedPoint.division_method
'error'
>>> FixedPoint.division_method('mamish')
>>> x / y
FixedPoint('0x2', signed=0, m=2, n=2, overflow='clamp', rounding='nearest', overflow_alert='error', mismatch_alert='warning', implicit_cast_alert='warning', str_base=16)
>>> float(_)
0.5

However, __floordiv__, __mod__, __divmod__, and their reflective and augmented methods can be adequately and completely defined for all cases.

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.

2 participants