Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parse komi correctly with SGF files from Fox Weiqi #11

Open
kevinsung opened this issue Jul 24, 2022 · 2 comments
Open

Parse komi correctly with SGF files from Fox Weiqi #11

kevinsung opened this issue Jul 24, 2022 · 2 comments
Assignees
Labels
good first issue Good for newcomers

Comments

@kevinsung
Copy link

Fox Weiqi (https://www.foxwq.com/) is a popular Go server. Unfortunately, it populates the komi field (KM) in a non-standard way for downloaded SGF files. An even game will have the komi listed as "375" instead of "7.5," causing Sabaki to parse the komi incorrectly. It could be argued that nothing should be done since Fox Weiqi is not following the SGF standard. However, adding a special case to parse these files would greatly improve Sabaki's usability. KaTrain does this and is able to parse the files. The code it uses is at
https://github.com/sanderland/katrain/blob/485b55c42df4dd1c77abf21eefc23c9a17d6a512/katrain/core/sgf_parser.py#L422-L430

@apetresc apetresc self-assigned this Aug 23, 2022
@apetresc apetresc added the good first issue Good for newcomers label Aug 23, 2022
@loarabia
Copy link

loarabia commented Aug 6, 2023

@yishn , are you interested in having someone take this on? I was considering a change for it just now.

My Proposal: I figured that it'd probably be best to make some form of "fox-helper.js" that has fixup code in it.

Why a separate function? This means that by default, sgf's parse functions are still strict in what they accept but users could also call the fixup on a tree of nodes after parsing is completed -- its an attempt to balance strictness vs. usability. I do worry about discoverability though so open to putting it directly in parse.

Why a separate file? If this is going to be its own helper, then this function would work on nodes like stringify does. All the other functions and files deal with different themes -- helper's functions take strings, parse's functions deal with tokens, files, buffers, and iterators.

I did consider following Postel's law and modifying _parseTokens and add the option to fixup fox's KM node as a default option. Open to feedback on this.

Option 1:

const sgf = require('./main')
const path = require('path')

let pathname = path.resolve(__dirname,'../tests/sgf/fox_go_server.sgf')
let rootNodes = sgf.parseFile(pathname)
let changedKomi = sgf.fixupFoxKomi(rootNodes) 
console.log(changedKomi) // true

Option 2:

const sgf = require('./main')
const path = require('path')

let pathname = path.resolve(__dirname,'../tests/sgf/fox_go_server.sgf')
let rootNodes = sgf.parseFile(pathname, {fixupFoxKomi:true)
console.log(rootNodes)  // KM node already adjusted

@loarabia
Copy link

loarabia commented Aug 6, 2023

More I look at it, I'm thinking to handle it like KaTrain does -- inside the parser directly.

loarabia added a commit to loarabia/sgf that referenced this issue Aug 7, 2023
This is just the functional part and two test files. Test cases coming
next as well as documentation.

Adjusting Komi on fox is added as an option you can pass to ParseToken.

This handles fox's komi in 3 cases by following kaTrain's fixup:
1. Handicap games it adjusts to 0.5 Komi
2. Even games using chinese rules it adjusts to 7.5 Komi (from 375)
3. From other rulesets it adjusts to (6.5 from 325)

Note: probably a bug here as some games have 0 as Komi and we shouldn't
add Komi in those cases.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Development

No branches or pull requests

3 participants