AlignmentResearch / KataGoVisualizer

MIT License
3 stars 1 forks source link

notebooks/iclr2022: Split plots.ipynb into separate notebooks #21

Closed tomtseng closed 1 year ago

tomtseng commented 1 year ago

Refactors plots.ipynb by splitting into separate notebooks.

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

netlify[bot] commented 1 year ago

Deploy Preview for goattack canceled.

Name Link
Latest commit ad4d769b0580ab56b6ad7c26b74d246071a835f6
Latest deploy log https://app.netlify.com/sites/goattack/deploys/638ffaaf5534ad00085b86fa
tomtseng commented 1 year ago

Addressed comments.

The notebooks currently set formatting parameters by setting globals in utils.set_plot_formatting(), and then overriding some components such as figure size on a per-cell basis. I was wondering why you chose to do this rather than using matplotlib style sheets and context managers like with plt.style.context(utils.STYLES['default'], utils.STYLES['1-col']).

I've added style sheets now. I didn't know about this feature of Matplotlib

Do we really want the PGF files to be comittted to this repo? I'd be inclined to add them to .gitignore and instead just commit them in the go-attack-paper repo, but open to arguments to the contrary.

Maybe it's mildly useful to check whether notebook changes have changed the plots, and it's less likely that someone will accidentally upload old plots to go-attack-paper. But removing them seems fine — the plots are visible in the notebooks anyway

tomtseng commented 1 year ago

I think the only resolved comment is this one on win-rate-vs-training-steps.ipynb:

The code in this particular notebook feels fundamental enough that we likely want to move it outside of a notebook into a Python module -- but that can be another PR.

I'll make a kanban task to clean up the notebooks more