Closed johankristiansson closed 5 years ago
Merging #30 into master will increase coverage by
1.59%
. The diff coverage is78.26%
.
@@ Coverage Diff @@
## master #30 +/- ##
==========================================
+ Coverage 86.49% 88.08% +1.59%
==========================================
Files 149 150 +1
Lines 1414 1444 +30
Branches 86 89 +3
==========================================
+ Hits 1223 1272 +49
+ Misses 191 172 -19
Impacted Files | Coverage Δ | |
---|---|---|
src/utils/compare-utils.ts | 97.29% <100%> (+87.29%) |
:arrow_up: |
src/utils/floatN.ts | 100% <100%> (ø) |
|
src/amount.ts | 51.85% <23.07%> (+8.14%) |
:arrow_up: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact)
,ø = not affected
,? = missing data
Powered by Codecov. Last update 21a46e1...79bf03f. Read the comment docs.
Nice work so far!
Is there an advantage to having a concept like Float54
over just having a comparison function? Something like compareNumbersWithLessBits(number1, number2, bitCountToIgnore)
. I guess if it is possible to store the Float54
value it might be possible to skip logic to create it each time a comparison should be done. It could be stored as part of Amount, however always calculating Float54
would be ineffective since comparison does not happen for all Amounts. Also I think the number of bits to ignore might be application specific. So perhaps it could always be dynamically calculated for each comparison and then a comparison function might be enough?
Currently the comparison function is located on the compare-utils.ts
module but this is not part of the public API (I think this is an internal module). I guess we need to add something to the Amount
module to surface this?
Once it is determined in which module the function should be it would be nice if you could add a jsdoc header for the function (the one that is part of the public API). If you run yarn gen-doc
it should then include the information from that header in the generated docs (which is available here).
I'd consider the Float54
concept just as a proof that this could be done.
It would be easy to parameterize the number of bits used for precision. If we decide to do that we should probably rename the concept to FloatN
where 0 >= N <= 51
so it will reflect the number of significant bits in the mantissa part of the floating point.
Then it could be typed in the following way:
type Precision = 0 | 1 | 2 | 3 | 4 | ... | 51 ;
interface Float<T extends Precision> {
readonly precision: T;
}
One way of adding this concept to Amount
API could be as an optional parameter to the compare functions:
function equals<T1 extends Quantity, T2 extends T1>(
left: Amount<T1>,
right: Amount<T2>,
precision?: Float<Precision>
)
The precision
parameter would then follow the comparison function all the way down to where the actual math happens and then use a parameterized version of the the current Float54
concept.
FloatN seems like a nice concept.
I get the Precision
type, it is a good way to limit which values you can use for precision. However I'm not sure I understand the purpose of the Float<T>
interface? I think equals
would be something like:
function equals<T1 extends Quantity, T2 extends T1>(
left: Amount<T1>,
right: Amount<T2>,
precision?: Precision
)
Theoretically there are (will be) multiple ways to perform equals
between two amounts.
We could either expose these ways as separate functions, or have combinations of them in the same function. Since amounts can have optional decimals specified, (1) is the same as having undefined decimals for both amounts. Also since decimals can be specified (1) would probably not make sense as a stand alone function (unless you consider the number of decimals as a formatting concern only). Option (2) is what we currently to in equals
. Adding bit precision we could expose (3) separately. However that would make (3) not look at number of decimals. So probably the most expected approach would be to do (2) in combination with optionally also doing (3) like you suggested. I guess the decimals should be considered first and then after that the bit precision should be applied.
Wrapping precision
in Float<T>
does not provide any technical usage - it is simply for clarifying that the precision is based on bits rather than decimals. If it's just a number it could be misunderstood as a decimal precision. Imo Float<T>
implies that bits are significant here.
Regardless of how we want to type precision
I think we are in an agreement of the implementation should work.
If precision
is undefined
the comparison should be evaluated as before.
If precision
is defined we should apply it after the decimals have been considered.
I will update the PR later today.
Closing this since #31 will allow for this functionallity to be solved outside of this package.
Proposal derived from an offline discussion with @jonaskello and @Jontem to implement amount (number) comparison using only the 54 most significant bits.
Tests have been added to show what kind of cases will benefit from comparing numbers using only 54 bits.