Support displaying score lead in analysis graph - #828
Conversation
|
This is a nice, clean implementation of adding a score lead graph. The keybinding (C-H) make it easy to toggle between the two modes. I have tested out commit [6f0751a], but I like it well enough that locally, I'm running a merge of Kevin's |
6f0751a to
8cc9b04
Compare
8cc9b04 to
5b469e3
Compare
|
Just a heads-up that I haven't ignored this PR either, it's just that this one requires much more thorough tests to be written (since currently Sabaki's integration/e2e tests cover engine analysis only shallowly). So I'm taking a detour through a different branch to beef up the test coverage there, and then use that to implement e2e tests for this one as well. So, working on this one too, should be ready maybe in about 12-24 hours. |
The score-lead feature persists a new SBKS property, so teach the Clean Markup drawer's win-rate removal group to drop it too, and relabel the option "Analysis data" since it now covers both engine metrics. The cleanmarkup.winrate setting key is left unchanged so existing preferences keep working.
Drive the rendered graph against an SGF carrying SBKV/SBKS values (no live engine): the position marker, the value strip's percent-vs-signed formatting, and that toggling the metric re-renders the data line at an analysed node. Registers the analysis-graph Playwright project.
The marker was gated on a truthiness check of data[currentIndex]; since the win-rate/score-lead series is numeric, a value of exactly 0 (a dead-even score lead, common, or 0% win rate) is falsy and hid the marker. Guard with != null instead, and add a regression test at an even position.
shouldComponentUpdate ignored analysisType, so toggling win rate <-> score lead while data[currentIndex] was equal across metrics (notably null at an unanalysed node such as the root) skipped the re-render and the curve never switched. Compare analysisType too, and add a regression test toggling at the root.
…of SBKV) The replayed KataGo endgame transcript carries scoreLead, and the score-lead feature writes it onto the node as SBKS alongside SBKV. Assert it lands as a finite, decisively positive Black lead, exercising the full GTP analyze -> parseAnalysis -> SBKS pipeline end-to-end.
The marker-at-0 fix guarded with != null, which still admits NaN: a malformed/non-numeric SGF property coerces via +x to NaN and rendered the marker at top: NaN%. Guard with Number.isFinite, which keeps the marker for a real 0 (even score lead / 0% win rate) but drops NaN. Adds a regression test (a node with a non-numeric SBKS) asserting the marker is hidden.
The variation-menu path wrote SBKS whenever scoreLead was != null, so a malformed (NaN) per-variation score lead was persisted as the literal string "NaN". Use Number.isFinite to match the analysis-update path's guard and to reject both null (absent) and NaN (malformed) without isFinite's null-coerces-to-0 pitfall.
5b469e3 to
7202cf2
Compare
|
Rebased against master and the new engine integration test suite that this warranted. @kevinsung can you please do a round of manual testing and make sure I haven't broken anything? You are more familiar with this functionality than I am :) |
I just tested it and it still works. Thanks for working on this! |
3 participants
Fixes #755 . Closes #823 .
This PR is an alternative to #823. I did explain the issues with that PR in the comments there, but since this feature is a high priority for me I went ahead and implemented it myself. In contrast to #823, which adds a score lead line on top of the existing graph, this PR makes the graph show either win rate or score lead, but not both, with the same style in either case. The metric (winrate or score lead) shown on the graph always matches the metric shown in the heatmap.
There are many variables in the code that use the term "winrate" which is no longer accurate after this change, since it can mean either "winrate" or "score lead". To keep the diff small, I did not change those variable names in this PR. Those variables should probably be renamed in a future PR.
Screenshots (second image taken after clicking "Score Lead" in first image):
