search

AlignmentResearch / KataGoVisualizer

MIT License
3 stars 1 forks source link

sgf-initial commit #4

Closed UFO-101 closed 1 year ago

UFO-101 commented 1 year ago

WIP

UFO-101 commented 1 year ago

Just some suggestions

Thanks for your comments. I'm afraid I'm making quite big changes still as this is a draft.

netlify[bot] commented 1 year ago

Deploy Preview for goattack ready!

Name Link
Latest commit cd84940ae9c4921071b5e21ef38a4209083dff7a
Latest deploy log https://app.netlify.com/sites/goattack/deploys/63743eb447306f0008441e1e
Deploy Preview https://deploy-preview-4--goattack.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

UFO-101 commented 1 year ago

@AdamGleave @norabelrose Warning! I rewrote history. I think this is pretty much ready to merge.

UFO-101 commented 1 year ago

This is a big PR so it would make sense to divide the work of reviewing.

@norabelrose Would you be able to review everything in sgf-viewer/. This hasn't changed too much since you last looked at it. @AdamGleave Would you be able to review everything in go_attack_utils/, notebooks/ and parsing_server/ (ie. everything else)? This is basically just the change that moves game_info.py into its own package.

I appreciate this could take some time and you both have other priorities at the moment. Let me know if you'd like me to ask someone else.

AdamGleave commented 1 year ago

Thanks Tom for reviewing my "part" of this!

@UFO-101 I'm wondering if it'd be more efficient to open a separate PR with the go_attack changes cherrypicked, and get that merged, then merge the sgf-viewer stuff? They seem largely independent from one another, and splitting it up minimizes "straggler" problem (both changes are bottlenecked on the slowest one to get reviewed).

UFO-101 commented 1 year ago

@UFO-101 I'm wondering if it'd be more efficient to open a separate PR with the go_attack changes cherrypicked, and get that merged, then merge the sgf-viewer stuff? They seem largely independent from one another, and splitting it up minimizes "straggler" problem (both changes are bottlenecked on the slowest one to get reviewed).

I spent a few minutes trying to separate the changes and ran into a bunch of conflicts. So right now I don't think it's worth the effort (having game_info.py in a separate package isn't a blocker for me), but happy to do it if you think it needs doing sooner.

review-notebook-app[bot] commented 1 year ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

UFO-101 commented 1 year ago

@tomtseng Thanks for your comments.