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

Fixed Point Value system #99

Merged
merged 13 commits into from
Oct 23, 2024
Merged

Conversation

soneryaldiz
Copy link
Contributor

@soneryaldiz soneryaldiz commented Oct 4, 2024

Description & Motivation

Introduce a value system to represent unsigned or signed fixed-point values following Q notation.

Related Issue(s)

Fixed point support

Testing

All constructors, methods and operators have unit tests.

Backwards-compatibility

Not a braking change.

Documentation

Added a dedicated page to document fixed-point arithmetic.

@mkorbel1
Copy link
Contributor

mkorbel1 commented Oct 4, 2024

For breadcrumbs, this is related: intel/rohd#470

@soneryaldiz soneryaldiz marked this pull request as ready for review October 4, 2024 16:47
@soneryaldiz
Copy link
Contributor Author

soneryaldiz commented Oct 4, 2024

@mkorbel1 , I will refactor this implementation based on your great comments:

  • Refactor class members to eliminate integer and fraction
  • Refactor operators to allow arbitrary integer and fraction width and to re-use LogicValue operators
  • Add exceptions converting to Dart double (no saturation nor rounding)

Copy link
Contributor

@mkorbel1 mkorbel1 left a comment

Choose a reason for hiding this comment

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

changes looking good!

@soneryaldiz
Copy link
Contributor Author

@mkorbel1 , this PR is ready for final review. I will add randomized testing for operators.

Copy link
Contributor

@mkorbel1 mkorbel1 left a comment

Choose a reason for hiding this comment

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

What do you think of the two remaining comments?

  • accessors for the fraction and integer portions
  • to/from FloatingPointValue

I don't think they are strictly necessary to add (or if we want them, necessarily in this PR), just closing out remaining comments.

lib/src/arithmetic/values/fixed_point_value.dart Outdated Show resolved Hide resolved
test/arithmetic/values/fixed_point_value_test.dart Outdated Show resolved Hide resolved
test/arithmetic/values/fixed_point_value_test.dart Outdated Show resolved Hide resolved
test/arithmetic/values/fixed_point_value_test.dart Outdated Show resolved Hide resolved
@soneryaldiz
Copy link
Contributor Author

Accessors for the fraction and integer portions: This is a bit tricky and needs to be defined clearly. Here is an example: Q0.4 does not have an integer part, yet it can represent -1. When one asks for integer portion, what should it return?

to/from FloatingPointValue: Given both fixed point and floating point offers to/from double, is there a value add to have dedicated methods in this class?

@mkorbel1
Copy link
Contributor

Accessors for the fraction and integer portions: This is a bit tricky and needs to be defined clearly. Here is an example: Q0.4 does not have an integer part, yet it can represent -1. When one asks for integer portion, what should it return?

Ah good point, ok then!

to/from FloatingPointValue: Given both fixed point and floating point offers to/from double, is there a value add to have dedicated methods in this class?

What about large fixed point and floating point values that don't fit in double?

@soneryaldiz
Copy link
Contributor Author

Let's revisit from/to floating point after the refactoring is completed. Resolved all other comments in the last commit.

Copy link
Contributor

@mkorbel1 mkorbel1 left a comment

Choose a reason for hiding this comment

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

Looks great!

@mkorbel1 mkorbel1 merged commit e7c9093 into intel:main Oct 23, 2024
4 checks passed
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