ui: consistent rate formatting #2278

norwnd · 2023-03-31T08:03:49Z

As a follow-up to #2174 we still have some rates in order book (& depth/candle charts) with trailing zeros:

in this PR I'm basically truncating those leading 0s, which works because those rates are already complying with their respective rate steps, so it looks more like this:

client/webserver/site/src/js/charts.ts

norwnd · 2023-03-31T08:14:03Z

client/webserver/site/src/js/doc.ts

    return threeSigFigs.format(v)
  }

+  static formatFourSigFigs (v: number) {
+    if (v >= 1000 || Math.round(v) === v) return oneFractionalDigit.format(v)
+    return fourSigFigs.format(v)
+  }
+
  static formatFiveSigFigs (v: number, prec?: number): string {
    if (v >= 10000) return intFormatter.format(Math.round(v))
    else if (v < 1e5) return fullPrecisionFormatter(prec ?? 8 /* rate encoding factor */).format(v)


I moved fourSigFigs func into doc.ts (and renamed it to formatFourSigFigs ) so it is accessible both in markets.ts and charts.ts, but turns out there already are formatThreeSigFigs and formatFiveSigFigs that are pretty similar but use slightly different formatting rules ... which seems somewhat weird to me. Can't we use same formatting for all of these ?

You're right. It is weird. We really need consistent guidelines for display of numerical values. As I recently pointed out elsewhere, I typically draw a distinction between "display" values and "data" values. For "display" values, I think sig figs is appropriate, especially for numbers that have decimals (but for large numbers, I wouldn't, for instance, truncate 1,234,567 to 1,230,000). I personally feel like nobody ever needs more than 3 significant figures for display values, but I would entertain arguments for 4 or 5 sig figs too. For "data" values, for which it might be necessary to validate the number against e.g. an order, the full value should always be displayed, out to the number of decimals derived from UnitInfo and/or RateStep.

One problem though is that Javascript seems to prioritize minimumSignificantDigits over maximumFractionalDigits. That's why we end up with values like 0.00000001000 for fourSigFigs.format(0.00000001). I guess a question I have is whether we can omit the minimumSignificantDigits specifier in all cases.

Right, that's exactly why we unfortunately have to do this (v >= 1000) else if (v < 1e5) business. The ability to use the significant digits and fraction digits options together effectively is considered an experiential option. #2208 (comment)

Generally though I find 4 sig figs for smaller values to be the sweet spot, but as you said whole numbers precision should not be lost, only fractional values.

I agree with all of the above,

For "data" values the full value should always be displayed

we can't address this ^ in general by simply changing how we are using/combining different rounding modifiers (significant digits and fraction digits) because JS Number itself can't represent what is actually arbitrary precision Decimal.

Maybe in practice we don't need to (if those cases when we don't get precise representation for order rate/qty/... are very rare on DEX, and we can adjust those significant digits and fraction digits modifiers such that they fit our use-cases most closely. Otherwise we need to figure out how to handle Decimals precisely (use some 3rd party lib to handle it for us, for example).

Alternatively, we can probably implement our own AtomicDecimal that will be represented as integer number of atoms (limited by Number.MAX_SAFE_INTEGER) convertable to/from string representation (as decimal) with specified precision (e.g. rate_step, perhaps trimming non-effectual 0s) also erroring when precise representation isn't possible. I think this is pretty much how dexc represents it.

As you point out, this is why we use integer representations of quantities and rates on the backend. These numbers find also their way to the frontend. Particularly with the rates, these whole numbers do not exceed 2^53 – 1 so there is no precision loss because the values fit into the IEEE 754 64-bit float's significand.

Only when we start doing math with these on the frontend can we run into trouble. When we divide the whole number "message rate" by the "rate encoding factor" (1e8), we get the decimal value. If the float string formatters are used with the appropriate "rate step" or even the full 8 fractional digits of precision, it is rounded so that there is no incorrect or misleading information presented (the order of precision loss is far smaller than the number of digits we retain and the formatters do not simply truncate). We only need to request the appropriate number of digits for a rate based on the market's rate step.

EDIT: but we do need to be smart about order of operations when doing mult/div with these floats. Keeping the magnitudes balanced is important.

norwnd · 2023-03-31T08:37:28Z

Note, there is also a room to improve qty displayed in order book (which on depth chart is trimmed to 1 digit after the dot, is it integer-only in practice ?):

but that won't work while we are showing Market orders in the book with qty in opposite units:

which is counterintuitive on its own and probably needs changing some time in future.

EDIT: It will work actually while we trim with rounding via fourSigFigs, it won't if we were to format exactly to rate step, e.g. *.xxx.

norwnd · 2023-04-01T06:42:29Z

Also note, switching to using fourSigFigs in this PR makes those rates displayed in the book subject to the same rounding caveat mentioned here.

norwnd · 2023-04-08T09:40:15Z

Reworked things in that last force-push,

1st commit applies these changes buck54321/dcrdex@2e0968c...buck54321:dcrdex:four-sig-figs

2nd commit:

is using those ^ formatters buck54321 wrote for "display" values (when rounding is acceptable), also changed 3 and 5 figure rounding to 4 everywhere
using "to rate-step" representation for "Your Orders" section (effectively addressing ui: display your orders rate in header #2281 (comment) issue)
applies exact "to-rate-step" representation to trim non-effectual 0s in orderbook table for rate (leaving qty formatting there as is)

in UI it looks like this now:

norwnd · 2023-04-08T09:44:01Z

client/webserver/site/src/js/doc.ts

+  static decodeRateStep (rateStepEnc: number, baseUnitInfo: UnitInfo, quoteUnitInfo: UnitInfo) {
+    const [qFactor, bFactor] = [quoteUnitInfo.conventional.conversionFactor, baseUnitInfo.conventional.conversionFactor]
+    return rateStepEnc / (RateEncodingFactor * qFactor / bFactor)
+  }


Maybe we could "decode" rate step in a simpler manner ? Just asking, in case I missed something.

Although on back-end ConventionalRateAlt does it similarly too, so I guess not.

I wish this were easier. I'm constantly forgetting that all asset's conversion factors are not 1e8 anymore.

Although I really don't know if this is the best order of operations. Elsewhere we've found this was best: https://github.com/decred/dcrdex/pull/1733/files#diff-8466f141b8e33a7e492b165ae7b6509808343604c759f3a0cc36e06fc33f13efR892
#1733 (comment)

Should we also adjust ConventionalRateAlt then to do something like this ?

return float64(msgRate) * (float64(baseFactor) / float64(quoteFactor)) / RateEncodingFactor

It's doing this currently:

return float64(msgRate) / RateEncodingFactor * float64(baseFactor) / float64(quoteFactor)

Not sure, but if we change the backend we should use big.Float for the math.

buck54321 · 2023-04-11T21:02:49Z

client/webserver/site/src/js/doc.ts

+      // which looks like a comma to me. ¯\_(ツ)_/¯
+      ['ar-EG', '123.45678', undefined, '١٢٣٫٥'],
+      ['ar-EG', '1234', undefined, '١٬٢٣٤٫٠'],
+      ['ar-EG', '0.12345', 3, '٠٫١٢٣']


This test is failing because you're not passing the "preciseFormatter" and fullPrecisionFormatter uses navigator.languages.

So it is test-specific issue (and using navigator.languages is what we want users to use in practice), right ? We can use something like this to accommodate this test (it passes now): 2577606

The reason I wanted to remove preciseFormatter from there is because it kinda duplicates maxDecimals param (according to my understanding) and it isn't clear what happens if preciseFormatter has precision set to anything different from maxDecimals value.

buck54321 · 2023-04-11T21:05:13Z

client/webserver/site/src/js/doc.ts

@@ -291,6 +296,15 @@ export default class Doc {
    return fullPrecisionFormatter(prec).format(value)
  }

+  static decodeRateStep (rateStepEnc: number, baseUnitInfo: UnitInfo, quoteUnitInfo: UnitInfo) {


This is not a "decoded" rate step, which would be a ratio of atomic units. This is a rate step expressed in conventional units. So a name like conventionalRateStep would be more fitting.

Got a bit confused by conventional and decoded, kinda assuming both to be same thing. Seems clear now, applied.

buck54321 · 2023-04-11T21:29:35Z

client/webserver/site/src/js/doc.ts

@@ -291,6 +296,15 @@ export default class Doc {
    return fullPrecisionFormatter(prec).format(value)
  }

+  static decodeRateStep (rateStepEnc: number, baseUnitInfo: UnitInfo, quoteUnitInfo: UnitInfo) {
+    const [qFactor, bFactor] = [quoteUnitInfo.conventional.conversionFactor, baseUnitInfo.conventional.conversionFactor]
+    return rateStepEnc / (RateEncodingFactor * qFactor / bFactor)


Pretty sure this is supposed to be bFactor / qFactor.

Fixed as described here.

buck54321 · 2023-04-11T23:30:55Z

client/webserver/site/src/js/doc.ts

+    const r = bui.conventional.conversionFactor / qui.conventional.conversionFactor
+    const convRate = encRate * r / RateEncodingFactor
+    const rateStepDigits = Doc.rateStepDigits(Doc.decodeRateStep(rateStepEnc, bui, qui))
+    return convRate.toFixed(rateStepDigits)


toFixed is not locale-aware. We can't use it.

Should be fixed here, we might also want to add some locale-aware tests for formatRateFullPrecision to convey the intent.

buck54321 · 2023-04-12T00:37:03Z

client/webserver/site/src/js/charts.ts

-    this.rateStep = rateStep / RateEncodingFactor * qFactor / bFactor
+    this.rateStep = Doc.decodeRateStep(rateStepEnc, baseUnitInfo, quoteUnitInfo)


A couple of things here. 1) it appears that this was wrong? See other comment on bFactor / qFactor. 2) this.rateStep should really be called this.conventionalRateStep, but not really your problem here, so whatever.

it appears that this was wrong? See other comment on bFactor / qFactor

Yep, it contradicts this:

func ConventionalRateAlt(msgRate uint64, baseFactor, quoteFactor uint64) float64 { return float64(msgRate) / RateEncodingFactor * float64(baseFactor) / float64(quoteFactor) }

note, in your diff provided below it still seems to be incorrect due to extra set of () used there, yet testFormatRateFullPrecision tests pass; I've added some more unit-tests there and addressed it, and it seems to be working correctly now.

this.rateStep should really be called this.conventionalRateStep

I would like to rename it, while we are at it, done here.

buck54321 · 2023-04-12T00:38:39Z

client/webserver/site/src/js/doc.ts

+   * formatRate formats rate to represent it exactly at rate step precision,
+   * trimming non-effectual zeros if there are any.
+   */
+  static formatRateToRateStep (encRate: number, bui: UnitInfo, qui: UnitInfo, rateStepEnc: number) {


We are formatting an encoded rate to a conventional rate. Can the method name reflect that? This is also full-precision, so maybe a name like formatRateFullPrecision would be more apt.

so maybe a name like formatRateFullPrecision would be more apt.

Yep, sounds good, full precision also implies that it is a conventional rate. Applied.

buck54321 · 2023-04-12T00:40:43Z

client/webserver/site/src/js/doc.ts

+  static rateStepDigits (rateStepDec: number) {
+    return Math.round(Math.log10(1 / rateStepDec))
+  }


You might be onto something here using the conventional ratestep to determine the number of decimals to show. But, the argument is not just decoded (rateStepDec), it's conventional, so the argument name should reflect that. Also, this is only used once. Do we really need a method for this?

But, the argument is not just decoded (rateStepDec), it's conventional, so the argument name should reflect that.

Renamed as TODO now.

Also, this is only used once. Do we really need a method for this?

Probably not, thought it might be re-used somewhere but seems unlikely, inlined.

buck54321 · 2023-04-12T00:44:45Z

client/webserver/site/src/js/doc.ts

+   * formatRate formats rate to represent it exactly at rate step precision,
+   * trimming non-effectual zeros if there are any.
+   */
+  static formatRateToRateStep (encRate: number, bui: UnitInfo, qui: UnitInfo, rateStepEnc: number) {


I've explored some tests to see if we're getting what we expect from this method.

buck54321/dcrdex@1a17cc4...buck54321:dcrdex:full-precision-rate-tests

Nice, I was lazy to figure out how to run those, now I did,

I'm not sure how to use npm run watch properly (and it seems to be putting significant load on my laptop), so I added npm run build_dev which is similar to npm run build but allows for accessing those tests ... if you don't have better suggestions for me on this.

And applied changes too: 090f42a

I'm not sure how to use npm run watch properly

I think you have to use it with --no-embed-site, but I'm not sure either tbh. It has been a while since I've done anything other than a full build.

norwnd · 2023-04-12T11:16:24Z

client/webserver/site/src/js/doc.ts

    ]

    for (const [encRate, rateStep, qFactor, bFactor, expEncoding] of tests) {
+      for (const k in fullPrecisionFormatters) delete fullPrecisionFormatters[k] // cleanup


need to clean up fullPrecisionFormatters because we might have wrong locales there due to other tests running previously, which breaks tests depending on run order

buck54321 · 2023-04-19T15:53:30Z

client/webserver/site/src/js/doc.ts

+    if (!locales) {
+      locales = navigator.languages as string[]
+    }


fmt = new Intl.NumberFormat(locales || navigator.languages as string[], {

buck54321 · 2023-04-19T15:55:43Z

client/webserver/site/src/js/doc.ts

+function formatSigFigsWithFormatters (sigFigs: number, intFormatter: Intl.NumberFormat, sigFigFormatter: Intl.NumberFormat, n: number, maxDecimals?: number, locales?: string | string[]): string {
+  if (n >= Math.round(Math.pow(10, sigFigs - 1))) return intFormatter.format(n)
+  const s = sigFigFormatter.format(n)
+  if (typeof maxDecimals !== 'number') return s
+  const fractional = sigFigFormatter.formatToParts(n).filter((part: Intl.NumberFormatPart) => part.type === 'fraction')[0].value
+  if (fractional.length <= maxDecimals) return s
+  return fullPrecisionFormatter(maxDecimals, locales).format(n)
+}


I suppose if we've adopted a four-only sig fig policy, we could simplify this a bit to get rid of the sigFigs argument..

Like that 0252336 ?

buck54321 · 2023-04-19T16:12:56Z

client/webserver/site/src/js/doc.ts

+    const r = bui.conventional.conversionFactor / qui.conventional.conversionFactor
+    const convRate = encRate * r / RateEncodingFactor
+    const conventionalRateStep = Doc.conventionalRateStep(rateStepEnc, bui, qui)
+    const rateStepDigits = Math.round(Math.log10(1 / conventionalRateStep))


I know this will work for rate steps of powers of 10. It doesn't seem to work for all rate steps though. For instance, paste this test into testFormatRateFullPrecision.

// UTXO assets but with a rate step that's not a perfect power of 10. // For a rate step of 500, a min rate would be e.g. rate step = 500. // 5e2 / 1e8 = 5e-6 = 0.000005 [5e2, 500, 1e8, 1e8, '0.000005'],

which results in

TEST FAILED: f(500, 100000000, 100000000, 500) => 0.00001 != 0.000005

Maybe do this instead.

Right, applied.

chappjc · 2023-04-20T01:30:07Z

Closing EOW. Please finalize.

chappjc

Whenever it's good with you, @buck54321.
Ideally a squash and rebase from @norwnd before merging, but I can handle that if need be.

norwnd · 2023-04-21T16:08:26Z

Squashed & rebased.

norwnd commented Mar 31, 2023

View reviewed changes

client/webserver/site/src/js/charts.ts Show resolved Hide resolved

norwnd force-pushed the ui-trim-non-effectual-0s-orderbook branch from caae876 to 45fe1bb Compare March 31, 2023 08:09

norwnd commented Mar 31, 2023

View reviewed changes

norwnd force-pushed the ui-trim-non-effectual-0s-orderbook branch from 45fe1bb to 1a17cc4 Compare April 8, 2023 09:36

norwnd mentioned this pull request Apr 8, 2023

ui: display your orders rate in header #2281

Merged

norwnd commented Apr 8, 2023

View reviewed changes

norwnd changed the title ~~ui: trim non-effectual 0s on markets page~~ ui: consistent rate formatting Apr 8, 2023

buck54321 requested changes Apr 12, 2023

View reviewed changes

norwnd commented Apr 12, 2023

View reviewed changes

buck54321 requested changes Apr 19, 2023

View reviewed changes

chappjc approved these changes Apr 20, 2023

View reviewed changes

buck54321 approved these changes Apr 21, 2023

View reviewed changes

ui: consistent rate formatting

2d0e493

norwnd force-pushed the ui-trim-non-effectual-0s-orderbook branch from 0252336 to 2d0e493 Compare April 21, 2023 16:06

chappjc merged commit 69afd5b into decred:master Apr 21, 2023
5 checks passed

		this.rateStep = rateStep / RateEncodingFactor * qFactor / bFactor
		this.rateStep = Doc.decodeRateStep(rateStepEnc, baseUnitInfo, quoteUnitInfo)

ui: consistent rate formatting #2278

ui: consistent rate formatting #2278

Conversation

norwnd commented Mar 31, 2023 • edited

Choose a reason for hiding this comment

buck54321 Apr 5, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

norwnd Apr 6, 2023 • edited

Choose a reason for hiding this comment

chappjc Apr 6, 2023 • edited

Choose a reason for hiding this comment

norwnd commented Mar 31, 2023 • edited

norwnd commented Apr 1, 2023

norwnd commented Apr 8, 2023

norwnd Apr 8, 2023 • edited

Choose a reason for hiding this comment

chappjc Apr 8, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

norwnd Apr 12, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

norwnd Apr 12, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

norwnd Apr 12, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

norwnd Apr 12, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chappjc commented Apr 20, 2023

chappjc left a comment

Choose a reason for hiding this comment

norwnd commented Apr 21, 2023

norwnd commented Mar 31, 2023 •

edited

buck54321 Apr 5, 2023 •

edited

norwnd Apr 6, 2023 •

edited

chappjc Apr 6, 2023 •

edited

norwnd commented Mar 31, 2023 •

edited

norwnd Apr 8, 2023 •

edited

chappjc Apr 8, 2023 •

edited

norwnd Apr 12, 2023 •

edited

norwnd Apr 12, 2023 •

edited

norwnd Apr 12, 2023 •

edited

norwnd Apr 12, 2023 •

edited