refactor: fixed point representation to use native signed integers #635
Conversation
There was a problem hiding this comment.
Hi, several things are missing here:
- I left comments in your code
fp16is not recognized by the compiler. Please have a look at the module cheat sheet.- The reimplementation of functions for
fp16needs to have tests
| } | ||
|
|
||
| fn HALF() -> i32 { | ||
| HALF |
There was a problem hiding this comment.
HALF constant is type u32 but the returned type is i32. you should change constant type
| } | ||
|
|
||
| fn ONE() -> i32 { | ||
| ONE |
There was a problem hiding this comment.
ONE constant is type u32 but the returned type is i32. you should change constant type
| } | ||
|
|
||
| fn MAX() -> i32 { | ||
| MAX |
There was a problem hiding this comment.
MAX constant is type u32 but the returned type is i32. you should change constant type
There was a problem hiding this comment.
Alright. I'll look at all these
|
|
||
| use orion::numbers::fixed_point::core::FixedTrait; | ||
| use orion::numbers::fixed_point::implementations::fp16x16::math::{ | ||
| core as core_math, trig, hyp, erf |
There was a problem hiding this comment.
these modules have functions implemented for FP16x16. Not the fp16 implementation. All the functions in that modules needs to be re-implemented for fp16 as well, otherwise you cannot import them in the fp16Impl
Pull Request type
This PR address fixed point representation to use native signed integers for FP16x16
Issue Number: #633
Does this introduce a breaking change?