SabakiHQ / Sabaki

An elegant Go board and SGF editor for a more civilized age.
https://sabaki.yichuanshen.de/
MIT License
2.41k stars 377 forks source link

Feature suggestion: Display territory analyze for KataGo #619

Open Nihisil opened 4 years ago

Nihisil commented 4 years ago

KataGo can assume the territory ownership through the game. It would be nice to display it on the board as Lizzie does:

image

Nihisil commented 4 years ago

Documentation about the feature in KataGo: https://github.com/lightvector/KataGo/blob/master/docs/GTP_Extensions.md

kata-analyze.ownership field

Chiefman commented 4 years ago

I have to agree with this feature request. As a beginner It would help my game to see the territory change dynamically

ivysrono commented 4 years ago

similar #589

yishn commented 4 years ago

Sabaki can't integrate with features that are only available in kata-analyze because kata-analyze doesn't conform to our analyze command standards.

lightvector commented 4 years ago

Except for the very fact of including more values to report, in what way does it not conform right now?

Is it the fact that winrate is reported as a float rather than multiplied by 100 and rounded to an integer? That's the only difference I can think of right now and it seems like a rather trivial one, so maybe I'm missing something obvious. If necessary, I can add an additional input option (following after color and interval) that changes this behavior.

Edit: There is also the fact that ownership is only output once, rather than per-move, because it applies to all board locations rather than just moves (for example, locations that are covered by stones or are illegal suicides). In reviewing KataGo's GTP-extension documentation (https://github.com/lightvector/KataGo/blob/master/docs/GTP_Extensions.md) I realized that this wasn't the most clearly documented, so I updated the documentation just now to be clearer about how ownership is placed within the "grammar tree" of the output format. But even if you don't support this, it shouldn't stop one from at least reporting values like the score, which are simply additional fields within the original info <key-value-pair> format.

yishn commented 4 years ago

Is it the fact that winrate is reported as a float rather than multiplied by 100 and rounded to an integer?

Exactly. Not only the winrate, but also lcb, prior are different from lz-analyze, albeit Sabaki is not using them. It may seem trivial, but I think it's important that we have some kind of standard that we adhere to.

As for ownership, it doesn't quite fit into the key value pair format, since the "value" actually consists of multiple values separated by spaces. You need to know the number of values before you're able to parse the whole directive correctly.

lightvector commented 4 years ago

Thanks for the clarifications! I think I understand. Specific thoughts:

What do you think?

(edit: minor clarifications, some formatting improvements, note about future-proofness)

yishn commented 4 years ago

Thanks for explaining your rationale! ownership as a "top-level property" next to info makes a lot of sense to me now. BTW, does ownership information really change so frequently that one needs to subscribe to it in real time? Seems to me that a separate GTP command could suffice.

About the winrate values, etc... I agree with you that using floats probably would have been a better choice.

Frankly, I don't like the idea of a scale-values-by option. It only transforms the question "Why should we parse kata-analyze differently from lz-analyze?" into "Why should we send the kata-analyze command differently from lz-analyze"? That would undermine the whole point of standardization. The name also begs the question, what values does the option scale exactly?

Obviously, changing the default behavior of (kata|lz)-analyze at this point is a bad idea since everyone is already depending on the corresponding formats. I thought about simply detecting whether the value has a . in it and treat it accordingly, however, the value 1 would be ambiguous and could mean 100% or 0.01% at the same time.

Now I'm also out of good ideas.

nemja commented 4 years ago

the question "Why should we parse kata-analyze differently from lz-analyze?

Isn't this still by far the smallest problem? Score estimates are only reported in the former, so some differences are unavoidable anyway.

yishn commented 4 years ago

@lightvector I've tested out a few scenarios; it seems it's unlikely that KataGo reports a board position with a 100% winrate, so I'm going forward with the idea of looking if a winrate has a . or not, so 100% needs to be reported as 1.0. Maybe KataGo already reports it like this? This will be backward compatible with both Leela Zero and KataGo.

I will update the documentation as follows:

  • winrate - If specified as an integer, it represents the win rate percentage times 100 of move, e.g. 9543 for 95.43%. If specified as a float, i.e. includes a ., it represents the win rate percentage given between 0.0 and 1.0.

What do you think?

lightvector commented 4 years ago

I constructed a board position where the winrate from kata-analyze on a move is exactly 100%, and currently it does look like the floating point formatter currently used by the code (std::cout, operator<<) will output it without a decimal point.

Depending on the presence of a decimal point for scaling feels a little fragile. Given that KataGo already fully conforms to Sabaki's analysis output format for both the "analyze" and "lz-analyze" commands, can we not simply just say either that "kata-analyze" needs a slightly different numeric parser, or to allow me to implement an optional flag that would bring it in line with the other two analysis commands on precisely the fields that currently overlap between the two?

The semantics of the command is rigidly documented (https://github.com/lightvector/KataGo/blob/master/docs/GTP_Extensions.md) and I plan to never break compatibility in any future changes, so there shouldn't need to be some dependence on subtle formatting details.

Sorry for my part in the mess. If I had realized that lz-analyze itself was based around something from Sabaki, I might have considered more carefully for output formatting. Until more recently I wasn't aware that it was anything other than a one-off Leela-Zero-specific command with no prior precedent, the same way GnuGo or Pachi have a ton of idiosyncratic extensions as well, so I didn't see the harm in picking a scaling that made it more natural to have higher precision, given that I wanted to add a bunch of other new values as well.

yishn commented 4 years ago

Maybe you can introduce a decimal point for 100% for the future? I could also forgo kata-analyze completely and you simply add scoreLead to the output of lz-analyze so Sabaki can use it, or, you introduce a new command analyze or sabaki-analyze that implements Sabaki's specification.

The syntax of lz-analyze is something that the Leela Zero community developed specifically for Lizzie, which I also closely followed, then adopted for Sabaki. I then took the liberty in documenting it in more detail.

My goal here is to avoid idiosyncratic GTP extensions and let any engine to utilize Sabaki's analysis visualization features without me having to implement engine specific logic continuously. I'm sure a consistent syntax will benefit everyone in the end.

lightvector commented 4 years ago

The decimal point is fragile and would be hacky to force. I don't think this is a good idea - relying on the way a float is written or trying to get it to format exactly right is going to be a source of potential bugs.

Introducing sabaki-analyze seems the cleanest if you want to have a single uniform API that all bots are supposed to target when supporting Sabaki, where as a fallback if the command doesn't exist, you can still check for and/or call lz-analyze which supports only a subset of those things (assuming you do add score/ownership to sabaki-analyze). One way or another, someone has to do the work of implementing logic continuously, but if you prefer that to be on the engine writer's side to support your GUI rather than you to support the engine that seems extremely reasonable.

One note if you're thinking about implementing ownership prediction reporting API: a toggle to turn it on/off in the bot itself is necessary, because unlike score-related outputs, which are just a single additional number, ownership predictions are a value per every board location, and computing and reporting it has nontrivial cost and will slow down the search, so it should be computed only if actually needed. If Sabaki is in a mode where it will not be displayed, the API should support a way to also tell the bot in the analysis command so that the bot can stop computing it. (kata-analyze indeed currently implements an argument-based method of controlling whether it reports ownership or not).

lightvector commented 4 years ago

Oh, does analyze already basically play the role sabaki-analyze? analyze is such a generic name that I was under the impression that it was some much more widespread standard that Leela Zero was implementing from some established GTP-extension protocol, rather than it being derived from Leela Zero, or having anything whatosever to do with Sabaki.

Benefit of modifying analyze or creating sabaki-analyze is that each time some new bot implements its own brand new analysis command, you don't have to check for and support yet another variation, even if it's merely changing the name of the command to call.

yishn commented 4 years ago

AFAIK there was no precedent to analyze before lz-analyze. It was just my attempt to formalize lz-analyze, which is already designed the way that it supports adding custom properties.

I'm fine with having a sabaki-analyze if people are too wary to just use analyze.

lightvector commented 4 years ago

Okay, well let me know what you want to do. If you're still really a fan of the float decimal point thing, I'll try to safeguard KataGo's output for next release. Otherwise, I'll add the appropriate fields in whatever specification you wish to either analyze or sabaki-analyze if you give me a specification for what fields you want in there and the precise format for those new fields (and/or ownership support).

yishn commented 4 years ago

I've thought about it, and I'd like to keep my specification like it is now (different parsing based on whether decimal point is available or not), because I do like specifying percentages as decimal numbers, and I also don't want to lose compatibility with lz-analyze. IMHO, breaking compatibility on this minor matter doesn't seem appropriate.

On your side, if you don't want to introduce a "hack" to have a decimal point for 1, you could add a new command analyze or sabaki-analyze that behaves just like kata-analyze with the exception that percentages are given by integers.

lightvector commented 4 years ago

The thing I don't like is that this choice deliberately introduces a bug that did not exist before between existing versions (namely, Sabaki post-decimal-point change and Kata v1.3.3), in the face of pre-existing documentation that warns that such an implementation may have such a bug (or at least, does not preclude that it may have a bug). But maybe it's so rare that it doesn't matter, and I agree compatibility is nice. Anyways, I'll update the floating point output in the next version then.

lightvector commented 4 years ago

And I'll modify the plain "analyze" command to also have the appropriate fields too, now that I know that this is a Sabaki-specific command rather than one that was decided on and fixed by someone else.

(Edit: Thanks!)

lightvector commented 4 years ago

It turns out to be a bit messy to ensure that floats always have decimal points for kata-analyze, because the the only convenient way of doing so in C++ also results in the output becoming unnecessarily verbose in other ways (also outputting extraneous trailing zeros on all values that have less than 6 digits of precision to bring them up to 6 digits, and things like that).

So I stuck with solely modifying analyze and genmove_analyze to have this change, but otherwise made them the same as kata-analyze. The documentation is now updated to reflect the current state of the master branch with this change, which should go out next release. https://github.com/lightvector/KataGo/blob/master/docs/GTP_Extensions.md

yishn commented 4 years ago

Thank you for the update!

lightvector commented 4 years ago

KataGo v.1.3.4 is released with these changes now. Let me know if there is anything else. Thanks! (https://github.com/lightvector/KataGo/releases/tag/v1.3.4)

benjaminvdb commented 4 years ago

Thanks @yishn and @lightvector for collaborating on improving Sabaki and KataGo interoperability! 🍾

hadim commented 2 years ago

Any news here?

zliu1022 commented 10 months ago

May I ask the progress? Such an amazing feature we are waiting for so long time. 😀 @yishn