Add Figure 5 code

[norabelrose]

Added the code to reproduce Figures 5a and 5b from the paper to notebooks/notebooks/iclr2022/plots.ipynb.

The code works on my local machine with LaTeX installed, but I don't actually know how to get it to work on rnn without having root access to install LaTeX there. I'm not sure how we've been handling that in the past.

[review-notebook-app[bot]]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[netlify[bot]]

✅ Deploy Preview for goattack canceled.

Name	Link
🔨 Latest commit	afaae126b9bdbb308d828c2050b3bb8248548840
🔍 Latest deploy log	https://app.netlify.com/sites/goattack/deploys/63ae3a87c9548e000981b9e4

[norabelrose]

Also the tests seem to be failing for reasons ostensibly unrelated to my PR? It says no configuration file provided: not found when running docker-compose up -d.

[tomtseng]

FYI I have a PR #21 splitting plots.ipynb into separate notebook, so you might want to also put your new code in a separate notebook to avoid merge conflicts, or you can comment in the PR saying that splitting plots.ipynb seems unnecessary

The code works on my local machine with LaTeX installed, but I don't actually know how to get it to work on rnn without having root access to install LaTeX there. I'm not sure how we've been handling that in the past.

I had the same issue — see notebooks/README.md, you can launch a docker container that has LaTeX installed and run the notebook inside the container

[UFO-101]

FYI If you rebase on main the tests will hopefully pass.

[review-notebook-app[bot]]

View / edit / reply to this conversation on ReviewNB

tomtseng commented on 2022-11-30T04:04:28Z ----------------------------------------------------------------

Line #6.        """Return (adv_win_rates, victim_win_rates, gt_adv_wins) as a tuple.

not clear what gt_adv_wins is without looking at the function body — we should either explain what it is or give it a clearer name

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

View / edit / reply to this conversation on ReviewNB

tomtseng commented on 2022-11-30T04:04:29Z ----------------------------------------------------------------

Line #27.            # By default we assume adversary is playing white; flip probs if needed

this comment seems either unnecessary or should be moved to when we look at the probs a few lines later

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

View / edit / reply to this conversation on ReviewNB

tomtseng commented on 2022-11-30T04:04:31Z ----------------------------------------------------------------

Line #38.            # The first node is the root, which has no move, so skip it

should this comment be at the point we pop the root instead?

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

View / edit / reply to this conversation on ReviewNB

tomtseng commented on 2022-11-30T04:04:32Z ----------------------------------------------------------------

Line #48.                node.set('C', f'{win:.2f} {loss:.2f} {draw:.2f} {score:.2f}')

why are we modifying the node?

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

View / edit / reply to this conversation on ReviewNB

tomtseng commented on 2022-11-30T04:04:33Z ----------------------------------------------------------------

Line #56.                    # so we clip them to [0.01, 0.99] to avoid numerical issues in the BCE

maybe clip to [0.005, 0.995] since KataGo outputting a probability of 0.00 should mean an unrounded probability of <0.005, and similarly for a probability of 1.00? doesn't really matter though

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

View / edit / reply to this conversation on ReviewNB

tomtseng commented on 2022-11-30T04:04:34Z ----------------------------------------------------------------

Line #16.        }

note: some of these configs are just copy-pasted from the very top cell. If PR #21 merges then these configs should be replaced with calling utils.set_plot_formatting()

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

View / edit / reply to this conversation on ReviewNB

tomtseng commented on 2022-11-30T04:04:35Z ----------------------------------------------------------------

Line #7.        smoothed_victim_bce_loss_std = np.convolve(victim_loss_std, kernel, mode='valid')

hmm ideally we should mention in the paper that the curve is smoothed and that the shaded interval represents +- standard dev. though maybe that takes up too much space in the caption.

BTW does the plot look weird if remove the smoothing?

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

View / edit / reply to this conversation on ReviewNB

tomtseng commented on 2022-11-30T04:04:36Z ----------------------------------------------------------------

Line #24.    fig = plt.figure(figsize=(2.6448 + 0.15, 2.6448), dpi=150)

if PR #21 merges then 2.6448 should be replaced with utils.TWO_COL_PLOT_WIDTH

_tomtseng commented on 2022-12-07T02:35:18Z_ ----------------------------------------------------------------

going to merge PR #21 in a few minutes — due to modifications in PR #21, now what's expected is to call the following at the top of the file:

import utils

plt.style.use(
    ["tableau-colorblind10", utils.get_style("default"), utils.get_style("2-col")]
)

and then you shouldn't need to specify figsize or dpi

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

View / edit / reply to this conversation on ReviewNB

tomtseng commented on 2022-11-30T04:04:37Z ----------------------------------------------------------------

Line #32.    plt.savefig('fig-5a.pgf', backend='pgf')

nit: it would be better to not base the filename upon the figure numbers in the paper, since it's possible that figure numbers will change in future edits to the paper

AdamGleave commented 1 year ago

FYI I have a PR #21 splitting plots.ipynb into separate notebook, so you might want to also put your new code in a separate notebook to avoid merge conflicts, or you can comment in the PR saying that splitting plots.ipynb seems unnecessary

We definitely want to split them up, am sure #21 will be merged in some form and sorry for the slow review there. I think it'd make sense to put this code in a new notebook.

tomtseng commented 1 year ago

going to merge PR #21 in a few minutes — due to modifications in PR #21, now what's expected is to call the following at the top of the file:

import utils

plt.style.use(
    ["tableau-colorblind10", utils.get_style("default"), utils.get_style("2-col")]
)

and then you shouldn't need to specify figsize or dpi

---

View entire conversation on ReviewNB

AdamGleave commented 1 year ago

@norabelrose looks like this PR needs to be updated to address merge conflict after #20 got merged. Let me/Tom know when it's ready for review.

norabelrose commented 1 year ago

Thanks Adam, looks good to me

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[review-notebook-app[bot]]

View / edit / reply to this conversation on ReviewNB

tomtseng commented on 2022-11-30T04:04:29Z ----------------------------------------------------------------

Line #27.            # By default we assume adversary is playing white; flip probs if needed

this comment seems either unnecessary or should be moved to when we look at the probs a few lines later

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

View / edit / reply to this conversation on ReviewNB

tomtseng commented on 2022-11-30T04:04:31Z ----------------------------------------------------------------

Line #38.            # The first node is the root, which has no move, so skip it

should this comment be at the point we pop the root instead?

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

View / edit / reply to this conversation on ReviewNB

tomtseng commented on 2022-11-30T04:04:32Z ----------------------------------------------------------------

Line #48.                node.set('C', f'{win:.2f} {loss:.2f} {draw:.2f} {score:.2f}')

why are we modifying the node?

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

View / edit / reply to this conversation on ReviewNB

tomtseng commented on 2022-11-30T04:04:33Z ----------------------------------------------------------------

Line #56.                    # so we clip them to [0.01, 0.99] to avoid numerical issues in the BCE

maybe clip to [0.005, 0.995] since KataGo outputting a probability of 0.00 should mean an unrounded probability of <0.005, and similarly for a probability of 1.00? doesn't really matter though

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

View / edit / reply to this conversation on ReviewNB

tomtseng commented on 2022-11-30T04:04:34Z ----------------------------------------------------------------

Line #16.        }

note: some of these configs are just copy-pasted from the very top cell. If PR #21 merges then these configs should be replaced with calling utils.set_plot_formatting()

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

View / edit / reply to this conversation on ReviewNB

tomtseng commented on 2022-11-30T04:04:35Z ----------------------------------------------------------------

Line #7.        smoothed_victim_bce_loss_std = np.convolve(victim_loss_std, kernel, mode='valid')

hmm ideally we should mention in the paper that the curve is smoothed and that the shaded interval represents +- standard dev. though maybe that takes up too much space in the caption.

BTW does the plot look weird if remove the smoothing?

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

View / edit / reply to this conversation on ReviewNB

tomtseng commented on 2022-11-30T04:04:36Z ----------------------------------------------------------------

Line #24.    fig = plt.figure(figsize=(2.6448 + 0.15, 2.6448), dpi=150)

if PR #21 merges then 2.6448 should be replaced with utils.TWO_COL_PLOT_WIDTH

_tomtseng commented on 2022-12-07T02:35:18Z_ ----------------------------------------------------------------

going to merge PR #21 in a few minutes — due to modifications in PR #21, now what's expected is to call the following at the top of the file:

import utils

plt.style.use(
    ["tableau-colorblind10", utils.get_style("default"), utils.get_style("2-col")]
)

and then you shouldn't need to specify figsize or dpi

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

View / edit / reply to this conversation on ReviewNB

tomtseng commented on 2022-11-30T04:04:37Z ----------------------------------------------------------------

Line #32.    plt.savefig('fig-5a.pgf', backend='pgf')

nit: it would be better to not base the filename upon the figure numbers in the paper, since it's possible that figure numbers will change in future edits to the paper

AdamGleave commented 1 year ago

FYI I have a PR #21 splitting plots.ipynb into separate notebook, so you might want to also put your new code in a separate notebook to avoid merge conflicts, or you can comment in the PR saying that splitting plots.ipynb seems unnecessary

We definitely want to split them up, am sure #21 will be merged in some form and sorry for the slow review there. I think it'd make sense to put this code in a new notebook.

tomtseng commented 1 year ago

going to merge PR #21 in a few minutes — due to modifications in PR #21, now what's expected is to call the following at the top of the file:

import utils

plt.style.use(
    ["tableau-colorblind10", utils.get_style("default"), utils.get_style("2-col")]
)

and then you shouldn't need to specify figsize or dpi

---

View entire conversation on ReviewNB

AdamGleave commented 1 year ago

@norabelrose looks like this PR needs to be updated to address merge conflict after #20 got merged. Let me/Tom know when it's ready for review.

norabelrose commented 1 year ago

Thanks Adam, looks good to me

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[review-notebook-app[bot]]

View / edit / reply to this conversation on ReviewNB

tomtseng commented on 2022-11-30T04:04:31Z ----------------------------------------------------------------

Line #38.            # The first node is the root, which has no move, so skip it

should this comment be at the point we pop the root instead?

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

View / edit / reply to this conversation on ReviewNB

tomtseng commented on 2022-11-30T04:04:32Z ----------------------------------------------------------------

Line #48.                node.set('C', f'{win:.2f} {loss:.2f} {draw:.2f} {score:.2f}')

why are we modifying the node?

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

View / edit / reply to this conversation on ReviewNB

tomtseng commented on 2022-11-30T04:04:33Z ----------------------------------------------------------------

Line #56.                    # so we clip them to [0.01, 0.99] to avoid numerical issues in the BCE

maybe clip to [0.005, 0.995] since KataGo outputting a probability of 0.00 should mean an unrounded probability of <0.005, and similarly for a probability of 1.00? doesn't really matter though

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

View / edit / reply to this conversation on ReviewNB

tomtseng commented on 2022-11-30T04:04:34Z ----------------------------------------------------------------

Line #16.        }

note: some of these configs are just copy-pasted from the very top cell. If PR #21 merges then these configs should be replaced with calling utils.set_plot_formatting()

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

View / edit / reply to this conversation on ReviewNB

tomtseng commented on 2022-11-30T04:04:35Z ----------------------------------------------------------------

Line #7.        smoothed_victim_bce_loss_std = np.convolve(victim_loss_std, kernel, mode='valid')

hmm ideally we should mention in the paper that the curve is smoothed and that the shaded interval represents +- standard dev. though maybe that takes up too much space in the caption.

BTW does the plot look weird if remove the smoothing?

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

View / edit / reply to this conversation on ReviewNB

tomtseng commented on 2022-11-30T04:04:36Z ----------------------------------------------------------------

Line #24.    fig = plt.figure(figsize=(2.6448 + 0.15, 2.6448), dpi=150)

if PR #21 merges then 2.6448 should be replaced with utils.TWO_COL_PLOT_WIDTH

_tomtseng commented on 2022-12-07T02:35:18Z_ ----------------------------------------------------------------

going to merge PR #21 in a few minutes — due to modifications in PR #21, now what's expected is to call the following at the top of the file:

import utils

plt.style.use(
    ["tableau-colorblind10", utils.get_style("default"), utils.get_style("2-col")]
)

and then you shouldn't need to specify figsize or dpi

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

View / edit / reply to this conversation on ReviewNB

tomtseng commented on 2022-11-30T04:04:37Z ----------------------------------------------------------------

Line #32.    plt.savefig('fig-5a.pgf', backend='pgf')

nit: it would be better to not base the filename upon the figure numbers in the paper, since it's possible that figure numbers will change in future edits to the paper

AdamGleave commented 1 year ago

FYI I have a PR #21 splitting plots.ipynb into separate notebook, so you might want to also put your new code in a separate notebook to avoid merge conflicts, or you can comment in the PR saying that splitting plots.ipynb seems unnecessary

We definitely want to split them up, am sure #21 will be merged in some form and sorry for the slow review there. I think it'd make sense to put this code in a new notebook.

tomtseng commented 1 year ago

going to merge PR #21 in a few minutes — due to modifications in PR #21, now what's expected is to call the following at the top of the file:

import utils

plt.style.use(
    ["tableau-colorblind10", utils.get_style("default"), utils.get_style("2-col")]
)

and then you shouldn't need to specify figsize or dpi

---

View entire conversation on ReviewNB

AdamGleave commented 1 year ago

@norabelrose looks like this PR needs to be updated to address merge conflict after #20 got merged. Let me/Tom know when it's ready for review.

norabelrose commented 1 year ago

Thanks Adam, looks good to me

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[review-notebook-app[bot]]

View / edit / reply to this conversation on ReviewNB

tomtseng commented on 2022-11-30T04:04:32Z ----------------------------------------------------------------

Line #48.                node.set('C', f'{win:.2f} {loss:.2f} {draw:.2f} {score:.2f}')

why are we modifying the node?

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

View / edit / reply to this conversation on ReviewNB

tomtseng commented on 2022-11-30T04:04:33Z ----------------------------------------------------------------

Line #56.                    # so we clip them to [0.01, 0.99] to avoid numerical issues in the BCE

maybe clip to [0.005, 0.995] since KataGo outputting a probability of 0.00 should mean an unrounded probability of <0.005, and similarly for a probability of 1.00? doesn't really matter though

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

View / edit / reply to this conversation on ReviewNB

tomtseng commented on 2022-11-30T04:04:34Z ----------------------------------------------------------------

Line #16.        }

note: some of these configs are just copy-pasted from the very top cell. If PR #21 merges then these configs should be replaced with calling utils.set_plot_formatting()

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

View / edit / reply to this conversation on ReviewNB

tomtseng commented on 2022-11-30T04:04:35Z ----------------------------------------------------------------

Line #7.        smoothed_victim_bce_loss_std = np.convolve(victim_loss_std, kernel, mode='valid')

hmm ideally we should mention in the paper that the curve is smoothed and that the shaded interval represents +- standard dev. though maybe that takes up too much space in the caption.

BTW does the plot look weird if remove the smoothing?

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

View / edit / reply to this conversation on ReviewNB

tomtseng commented on 2022-11-30T04:04:36Z ----------------------------------------------------------------

Line #24.    fig = plt.figure(figsize=(2.6448 + 0.15, 2.6448), dpi=150)

if PR #21 merges then 2.6448 should be replaced with utils.TWO_COL_PLOT_WIDTH

_tomtseng commented on 2022-12-07T02:35:18Z_ ----------------------------------------------------------------

going to merge PR #21 in a few minutes — due to modifications in PR #21, now what's expected is to call the following at the top of the file:

import utils

plt.style.use(
    ["tableau-colorblind10", utils.get_style("default"), utils.get_style("2-col")]
)

and then you shouldn't need to specify figsize or dpi

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

View / edit / reply to this conversation on ReviewNB

tomtseng commented on 2022-11-30T04:04:37Z ----------------------------------------------------------------

Line #32.    plt.savefig('fig-5a.pgf', backend='pgf')

nit: it would be better to not base the filename upon the figure numbers in the paper, since it's possible that figure numbers will change in future edits to the paper

AdamGleave commented 1 year ago

FYI I have a PR #21 splitting plots.ipynb into separate notebook, so you might want to also put your new code in a separate notebook to avoid merge conflicts, or you can comment in the PR saying that splitting plots.ipynb seems unnecessary

We definitely want to split them up, am sure #21 will be merged in some form and sorry for the slow review there. I think it'd make sense to put this code in a new notebook.

tomtseng commented 1 year ago

going to merge PR #21 in a few minutes — due to modifications in PR #21, now what's expected is to call the following at the top of the file:

import utils

plt.style.use(
    ["tableau-colorblind10", utils.get_style("default"), utils.get_style("2-col")]
)

and then you shouldn't need to specify figsize or dpi

---

View entire conversation on ReviewNB

AdamGleave commented 1 year ago

@norabelrose looks like this PR needs to be updated to address merge conflict after #20 got merged. Let me/Tom know when it's ready for review.

norabelrose commented 1 year ago

Thanks Adam, looks good to me

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[review-notebook-app[bot]]

View / edit / reply to this conversation on ReviewNB

tomtseng commented on 2022-11-30T04:04:33Z ----------------------------------------------------------------

Line #56.                    # so we clip them to [0.01, 0.99] to avoid numerical issues in the BCE

maybe clip to [0.005, 0.995] since KataGo outputting a probability of 0.00 should mean an unrounded probability of <0.005, and similarly for a probability of 1.00? doesn't really matter though

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

View / edit / reply to this conversation on ReviewNB

tomtseng commented on 2022-11-30T04:04:34Z ----------------------------------------------------------------

Line #16.        }

note: some of these configs are just copy-pasted from the very top cell. If PR #21 merges then these configs should be replaced with calling utils.set_plot_formatting()

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

View / edit / reply to this conversation on ReviewNB

tomtseng commented on 2022-11-30T04:04:35Z ----------------------------------------------------------------

Line #7.        smoothed_victim_bce_loss_std = np.convolve(victim_loss_std, kernel, mode='valid')

hmm ideally we should mention in the paper that the curve is smoothed and that the shaded interval represents +- standard dev. though maybe that takes up too much space in the caption.

BTW does the plot look weird if remove the smoothing?

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

View / edit / reply to this conversation on ReviewNB

tomtseng commented on 2022-11-30T04:04:36Z ----------------------------------------------------------------

Line #24.    fig = plt.figure(figsize=(2.6448 + 0.15, 2.6448), dpi=150)

if PR #21 merges then 2.6448 should be replaced with utils.TWO_COL_PLOT_WIDTH

_tomtseng commented on 2022-12-07T02:35:18Z_ ----------------------------------------------------------------

going to merge PR #21 in a few minutes — due to modifications in PR #21, now what's expected is to call the following at the top of the file:

import utils

plt.style.use(
    ["tableau-colorblind10", utils.get_style("default"), utils.get_style("2-col")]
)

and then you shouldn't need to specify figsize or dpi

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

View / edit / reply to this conversation on ReviewNB

tomtseng commented on 2022-11-30T04:04:37Z ----------------------------------------------------------------

Line #32.    plt.savefig('fig-5a.pgf', backend='pgf')

nit: it would be better to not base the filename upon the figure numbers in the paper, since it's possible that figure numbers will change in future edits to the paper

AdamGleave commented 1 year ago

FYI I have a PR #21 splitting plots.ipynb into separate notebook, so you might want to also put your new code in a separate notebook to avoid merge conflicts, or you can comment in the PR saying that splitting plots.ipynb seems unnecessary

We definitely want to split them up, am sure #21 will be merged in some form and sorry for the slow review there. I think it'd make sense to put this code in a new notebook.

tomtseng commented 1 year ago

going to merge PR #21 in a few minutes — due to modifications in PR #21, now what's expected is to call the following at the top of the file:

import utils

plt.style.use(
    ["tableau-colorblind10", utils.get_style("default"), utils.get_style("2-col")]
)

and then you shouldn't need to specify figsize or dpi

---

View entire conversation on ReviewNB

AdamGleave commented 1 year ago

@norabelrose looks like this PR needs to be updated to address merge conflict after #20 got merged. Let me/Tom know when it's ready for review.

norabelrose commented 1 year ago

Thanks Adam, looks good to me

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[review-notebook-app[bot]]

View / edit / reply to this conversation on ReviewNB

tomtseng commented on 2022-11-30T04:04:34Z ----------------------------------------------------------------

Line #16.        }

note: some of these configs are just copy-pasted from the very top cell. If PR #21 merges then these configs should be replaced with calling utils.set_plot_formatting()

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

View / edit / reply to this conversation on ReviewNB

tomtseng commented on 2022-11-30T04:04:35Z ----------------------------------------------------------------

Line #7.        smoothed_victim_bce_loss_std = np.convolve(victim_loss_std, kernel, mode='valid')

hmm ideally we should mention in the paper that the curve is smoothed and that the shaded interval represents +- standard dev. though maybe that takes up too much space in the caption.

BTW does the plot look weird if remove the smoothing?

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

View / edit / reply to this conversation on ReviewNB

tomtseng commented on 2022-11-30T04:04:36Z ----------------------------------------------------------------

Line #24.    fig = plt.figure(figsize=(2.6448 + 0.15, 2.6448), dpi=150)

if PR #21 merges then 2.6448 should be replaced with utils.TWO_COL_PLOT_WIDTH

_tomtseng commented on 2022-12-07T02:35:18Z_ ----------------------------------------------------------------

going to merge PR #21 in a few minutes — due to modifications in PR #21, now what's expected is to call the following at the top of the file:

import utils

plt.style.use(
    ["tableau-colorblind10", utils.get_style("default"), utils.get_style("2-col")]
)

and then you shouldn't need to specify figsize or dpi

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

View / edit / reply to this conversation on ReviewNB

tomtseng commented on 2022-11-30T04:04:37Z ----------------------------------------------------------------

Line #32.    plt.savefig('fig-5a.pgf', backend='pgf')

nit: it would be better to not base the filename upon the figure numbers in the paper, since it's possible that figure numbers will change in future edits to the paper

AdamGleave commented 1 year ago

FYI I have a PR #21 splitting plots.ipynb into separate notebook, so you might want to also put your new code in a separate notebook to avoid merge conflicts, or you can comment in the PR saying that splitting plots.ipynb seems unnecessary

We definitely want to split them up, am sure #21 will be merged in some form and sorry for the slow review there. I think it'd make sense to put this code in a new notebook.

tomtseng commented 1 year ago

going to merge PR #21 in a few minutes — due to modifications in PR #21, now what's expected is to call the following at the top of the file:

import utils

plt.style.use(
    ["tableau-colorblind10", utils.get_style("default"), utils.get_style("2-col")]
)

and then you shouldn't need to specify figsize or dpi

---

View entire conversation on ReviewNB

AdamGleave commented 1 year ago

@norabelrose looks like this PR needs to be updated to address merge conflict after #20 got merged. Let me/Tom know when it's ready for review.

norabelrose commented 1 year ago

Thanks Adam, looks good to me

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[review-notebook-app[bot]]

View / edit / reply to this conversation on ReviewNB

tomtseng commented on 2022-11-30T04:04:35Z ----------------------------------------------------------------

Line #7.        smoothed_victim_bce_loss_std = np.convolve(victim_loss_std, kernel, mode='valid')

hmm ideally we should mention in the paper that the curve is smoothed and that the shaded interval represents +- standard dev. though maybe that takes up too much space in the caption.

BTW does the plot look weird if remove the smoothing?

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

View / edit / reply to this conversation on ReviewNB

tomtseng commented on 2022-11-30T04:04:36Z ----------------------------------------------------------------

Line #24.    fig = plt.figure(figsize=(2.6448 + 0.15, 2.6448), dpi=150)

if PR #21 merges then 2.6448 should be replaced with utils.TWO_COL_PLOT_WIDTH

_tomtseng commented on 2022-12-07T02:35:18Z_ ----------------------------------------------------------------

going to merge PR #21 in a few minutes — due to modifications in PR #21, now what's expected is to call the following at the top of the file:

import utils

plt.style.use(
    ["tableau-colorblind10", utils.get_style("default"), utils.get_style("2-col")]
)

and then you shouldn't need to specify figsize or dpi

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

View / edit / reply to this conversation on ReviewNB

tomtseng commented on 2022-11-30T04:04:37Z ----------------------------------------------------------------

Line #32.    plt.savefig('fig-5a.pgf', backend='pgf')

nit: it would be better to not base the filename upon the figure numbers in the paper, since it's possible that figure numbers will change in future edits to the paper

AdamGleave commented 1 year ago

FYI I have a PR #21 splitting plots.ipynb into separate notebook, so you might want to also put your new code in a separate notebook to avoid merge conflicts, or you can comment in the PR saying that splitting plots.ipynb seems unnecessary

We definitely want to split them up, am sure #21 will be merged in some form and sorry for the slow review there. I think it'd make sense to put this code in a new notebook.

tomtseng commented 1 year ago

going to merge PR #21 in a few minutes — due to modifications in PR #21, now what's expected is to call the following at the top of the file:

import utils

plt.style.use(
    ["tableau-colorblind10", utils.get_style("default"), utils.get_style("2-col")]
)

and then you shouldn't need to specify figsize or dpi

---

View entire conversation on ReviewNB

AdamGleave commented 1 year ago

@norabelrose looks like this PR needs to be updated to address merge conflict after #20 got merged. Let me/Tom know when it's ready for review.

norabelrose commented 1 year ago

Thanks Adam, looks good to me

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[review-notebook-app[bot]]

View / edit / reply to this conversation on ReviewNB

tomtseng commented on 2022-11-30T04:04:36Z ----------------------------------------------------------------

Line #24.    fig = plt.figure(figsize=(2.6448 + 0.15, 2.6448), dpi=150)

if PR #21 merges then 2.6448 should be replaced with utils.TWO_COL_PLOT_WIDTH

_tomtseng commented on 2022-12-07T02:35:18Z_ ----------------------------------------------------------------

going to merge PR #21 in a few minutes — due to modifications in PR #21, now what's expected is to call the following at the top of the file:

import utils

plt.style.use(
    ["tableau-colorblind10", utils.get_style("default"), utils.get_style("2-col")]
)

and then you shouldn't need to specify figsize or dpi

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

View / edit / reply to this conversation on ReviewNB

tomtseng commented on 2022-11-30T04:04:37Z ----------------------------------------------------------------

Line #32.    plt.savefig('fig-5a.pgf', backend='pgf')

nit: it would be better to not base the filename upon the figure numbers in the paper, since it's possible that figure numbers will change in future edits to the paper

AdamGleave commented 1 year ago

FYI I have a PR #21 splitting plots.ipynb into separate notebook, so you might want to also put your new code in a separate notebook to avoid merge conflicts, or you can comment in the PR saying that splitting plots.ipynb seems unnecessary

We definitely want to split them up, am sure #21 will be merged in some form and sorry for the slow review there. I think it'd make sense to put this code in a new notebook.

tomtseng commented 1 year ago

going to merge PR #21 in a few minutes — due to modifications in PR #21, now what's expected is to call the following at the top of the file:

import utils

plt.style.use(
    ["tableau-colorblind10", utils.get_style("default"), utils.get_style("2-col")]
)

and then you shouldn't need to specify figsize or dpi

---

View entire conversation on ReviewNB

AdamGleave commented 1 year ago

@norabelrose looks like this PR needs to be updated to address merge conflict after #20 got merged. Let me/Tom know when it's ready for review.

norabelrose commented 1 year ago

Thanks Adam, looks good to me

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[review-notebook-app[bot]]

View / edit / reply to this conversation on ReviewNB

tomtseng commented on 2022-11-30T04:04:37Z ----------------------------------------------------------------

Line #32.    plt.savefig('fig-5a.pgf', backend='pgf')

nit: it would be better to not base the filename upon the figure numbers in the paper, since it's possible that figure numbers will change in future edits to the paper

AdamGleave commented 1 year ago

FYI I have a PR #21 splitting plots.ipynb into separate notebook, so you might want to also put your new code in a separate notebook to avoid merge conflicts, or you can comment in the PR saying that splitting plots.ipynb seems unnecessary

We definitely want to split them up, am sure #21 will be merged in some form and sorry for the slow review there. I think it'd make sense to put this code in a new notebook.

tomtseng commented 1 year ago

going to merge PR #21 in a few minutes — due to modifications in PR #21, now what's expected is to call the following at the top of the file:

import utils

plt.style.use(
    ["tableau-colorblind10", utils.get_style("default"), utils.get_style("2-col")]
)

and then you shouldn't need to specify figsize or dpi

---

View entire conversation on ReviewNB

AdamGleave commented 1 year ago

@norabelrose looks like this PR needs to be updated to address merge conflict after #20 got merged. Let me/Tom know when it's ready for review.

norabelrose commented 1 year ago

Thanks Adam, looks good to me

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[AdamGleave]

FYI I have a PR #21 splitting plots.ipynb into separate notebook, so you might want to also put your new code in a separate notebook to avoid merge conflicts, or you can comment in the PR saying that splitting plots.ipynb seems unnecessary

We definitely want to split them up, am sure #21 will be merged in some form and sorry for the slow review there. I think it'd make sense to put this code in a new notebook.

[tomtseng]

going to merge PR #21 in a few minutes — due to modifications in PR #21, now what's expected is to call the following at the top of the file:

import utils

plt.style.use(
    ["tableau-colorblind10", utils.get_style("default"), utils.get_style("2-col")]
)

and then you shouldn't need to specify figsize or dpi

---

View entire conversation on ReviewNB

[AdamGleave]

@norabelrose looks like this PR needs to be updated to address merge conflict after #20 got merged. Let me/Tom know when it's ready for review.

[norabelrose]

Thanks Adam, looks good to me