Skip to content

Add division operator to close Issue #2#9

Open
johnMamish wants to merge 4 commits intoSEL-FOSS:masterfrom
johnMamish:master
Open

Add division operator to close Issue #2#9
johnMamish wants to merge 4 commits intoSEL-FOSS:masterfrom
johnMamish:master

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