Refactoring of FloatingPointValue constructors for ease of use and extension
#110
Conversation
| sign: LogicValue.ofBigInt(sign ? BigInt.one : BigInt.zero, 1), | ||
| exponent: LogicValue.ofBigInt(BigInt.from(exponent), exponentWidth), | ||
| mantissa: | ||
| LogicValue.ofBigInt(BigInt.from(mantissa), mantissaWidth)); | ||
|
|
||
| /// Construct a [FloatingPointValue] from a Logic word | ||
| factory FloatingPointValue.fromLogic( |
There was a problem hiding this comment.
Why is this called fromLogic instead of fromLogicValue? Also, the comment says "word"? Also, this shouldn't be a factory?
There was a problem hiding this comment.
I'm struggling with having both factory and non-factory constructors. For example, fromDouble() is a factory constructor as it does a lot of work before it can initialize fields. But that forces sub-classes to also have a factory method that first checks for some things and then calls the base factory method, possibly then checking for other things.
|
|
||
| /// Numeric conversion of a [FloatingPoint8E4M3Value] from a host double | ||
| factory FloatingPoint8E4M3Value.fromDouble(double inDouble) { | ||
| if ((inDouble > maxValue) | (inDouble < minValue)) { |
There was a problem hiding this comment.
can we move this consideration of max/min into the base fromDouble in FloatingPointLogic?
There was a problem hiding this comment.
This is specific to E4M3. it's also wrong, so I have fixed it (it fails for 0.0 and negatives).
lib/src/arithmetic/floating_point/floating_point_values/floating_point_bf16_value.dart
Outdated
Show resolved
Hide resolved
| /// Factory (static) constructor of a [FloatingPointValue] from | ||
| /// sign, mantissa and exponent | ||
| factory FloatingPointValue( | ||
| /// A Map from the (exponentWidth, mantissaWidth) pair to the |
There was a problem hiding this comment.
I feel like we should update this doc comment to explain how it's used and how people can leverage it to override default behavior
There was a problem hiding this comment.
I am not sure of the utility of the map. We can work without it for now. I think if people are accessing the constructors from the base class, they won't get the special behavior of the sub-class constructors that I need to add. We need to figure out what we want here, as FPV(e=4,m=3) is not going to be the same type as FP8E4M3 since the latter does not represent infinity.
There was a problem hiding this comment.
Unfortunately the test for E4M3 calls the base class constructor and does not validate the fromDouble routine. So we have bugs in the very basic tests for the subtypes I need to check.
|
Yes, this is looking a lot better! |
Description & Motivation
Making it easier to extend
FloatingPointValueand reuse constructors, removingfactorys, etc.Related Issue(s)
N/A
Testing
Existing tests should cover.
Backwards-compatibility
No
Documentation
No