Score margin plots

[kellinpelrine]

Not sure about the y axis label "Score margin (adv. perspective)", if anyone has a better idea let me know please.

Adding plots to overleaf in appendix E.1.

[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 ready!

Name	Link
🔨 Latest commit	e7338b199a445a75301e96b66bdd64c2e80dc8f9
🔍 Latest deploy log	https://app.netlify.com/sites/goattack/deploys/63d75943e87e8e000af2f5c7
😎 Deploy Preview	https://deploy-preview-38--goattack.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

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

[review-notebook-app[bot]]

View / edit / reply to this conversation on ReviewNB

AdamGleave commented on 2023-01-03T01:58:55Z ----------------------------------------------------------------

Line #73.        elif 'scorediff' in wr_or_scorediff:

This is slightly brittle in that if wr_or_scorediff == 'scorediff_advwon' or some similar typo it'll just silently fail as if it was equal to 'scorediff'.There's also no error handling for if wr_or_scorediff is something totally bogus like 'foobar' . You could make it wr_or_scorediff in ('scorediff', 'scorediff_advwin', 'scorediff_advlose'): and then have a catch-all else that raises an exception?

_kellinpelrine commented on 2023-01-03T02:48:47Z_ ----------------------------------------------------------------

There's already an assert statement at the beginning (line 53) that will throw an error if wr_or_scorediff is wrong, i.e.

assert wr_or_scorediff in ['wr','scorediff','scorediff_advwin','scorediff_advlose']

Does this cover what you have in mind or still better to do it differently?

_AdamGleave commented on 2023-01-03T02:54:31Z_ ----------------------------------------------------------------

Ah, missed that, yeah how you have it now is fine then.

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

View / edit / reply to this conversation on ReviewNB

AdamGleave commented on 2023-01-03T01:58:56Z ----------------------------------------------------------------

Line #131.                target_value = below_value + (highlighted_point_step - below_idx) * ( (above_value - below_value) / (above_idx - below_idx) )

This is linearly interpolating between the points immediately before and after target value? Might want to add a comment to that effect.

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

View / edit / reply to this conversation on ReviewNB

AdamGleave commented on 2023-01-03T01:58:56Z ----------------------------------------------------------------

Line #1.    fig, _ = plot_training(

These cells are all the same except wr_or_scorediff so I'd be tempted to replace them with a single and a for loop. It's a bit less visually pleasing in the Jupyter notebook itself to have all the plots in one cell, but we're mostly using it for the PGF output and the plots are still distinguishable from the axes. (Same comment applies for the group of 4 cells for the cyclic adversary run below.)

AdamGleave commented 1 year ago

Actually, one comment not on the notebook itself but on the inclusion of the figures in the paper, for the figures restricted to adversary won/lost you've used the full-size figure but downscaled it. Can you switch those to the 2col style instead then? You can do this with a context manager like:

with plt.style.context(utils.get_style("2-col")):

Alternatively you could just include them full-size in the paper. Both seem OK -- it's an appendix!

Generally easier to read if font size is consistent throughout figures and not depend on the size of the figure itself.

kellinpelrine commented 1 year ago

There's already an assert statement at the beginning (line 53) that will throw an error if wr_or_scorediff is wrong, i.e.

assert wr_or_scorediff in ['wr','scorediff','scorediff_advwin','scorediff_advlose']

Does this cover what you have in mind or still better to do it differently?

---

View entire conversation on ReviewNB

kellinpelrine commented 1 year ago

LGTM, just some minor suggested changes to make.

Sounds good, replied with question on one of them, will make the others and then merge.

AdamGleave commented 1 year ago

Ah, missed that, yeah how you have it now is fine then.

---

View entire conversation on ReviewNB

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

[review-notebook-app[bot]]

View / edit / reply to this conversation on ReviewNB

AdamGleave commented on 2023-01-03T01:58:56Z ----------------------------------------------------------------

Line #131.                target_value = below_value + (highlighted_point_step - below_idx) * ( (above_value - below_value) / (above_idx - below_idx) )

This is linearly interpolating between the points immediately before and after target value? Might want to add a comment to that effect.

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

View / edit / reply to this conversation on ReviewNB

AdamGleave commented on 2023-01-03T01:58:56Z ----------------------------------------------------------------

Line #1.    fig, _ = plot_training(

These cells are all the same except wr_or_scorediff so I'd be tempted to replace them with a single and a for loop. It's a bit less visually pleasing in the Jupyter notebook itself to have all the plots in one cell, but we're mostly using it for the PGF output and the plots are still distinguishable from the axes. (Same comment applies for the group of 4 cells for the cyclic adversary run below.)

AdamGleave commented 1 year ago

Actually, one comment not on the notebook itself but on the inclusion of the figures in the paper, for the figures restricted to adversary won/lost you've used the full-size figure but downscaled it. Can you switch those to the 2col style instead then? You can do this with a context manager like:

with plt.style.context(utils.get_style("2-col")):

Alternatively you could just include them full-size in the paper. Both seem OK -- it's an appendix!

Generally easier to read if font size is consistent throughout figures and not depend on the size of the figure itself.

kellinpelrine commented 1 year ago

There's already an assert statement at the beginning (line 53) that will throw an error if wr_or_scorediff is wrong, i.e.

assert wr_or_scorediff in ['wr','scorediff','scorediff_advwin','scorediff_advlose']

Does this cover what you have in mind or still better to do it differently?

---

View entire conversation on ReviewNB

kellinpelrine commented 1 year ago

LGTM, just some minor suggested changes to make.

Sounds good, replied with question on one of them, will make the others and then merge.

AdamGleave commented 1 year ago

Ah, missed that, yeah how you have it now is fine then.

---

View entire conversation on ReviewNB

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

[review-notebook-app[bot]]

View / edit / reply to this conversation on ReviewNB

AdamGleave commented on 2023-01-03T01:58:56Z ----------------------------------------------------------------

Line #1.    fig, _ = plot_training(

These cells are all the same except wr_or_scorediff so I'd be tempted to replace them with a single and a for loop. It's a bit less visually pleasing in the Jupyter notebook itself to have all the plots in one cell, but we're mostly using it for the PGF output and the plots are still distinguishable from the axes. (Same comment applies for the group of 4 cells for the cyclic adversary run below.)

AdamGleave commented 1 year ago

Actually, one comment not on the notebook itself but on the inclusion of the figures in the paper, for the figures restricted to adversary won/lost you've used the full-size figure but downscaled it. Can you switch those to the 2col style instead then? You can do this with a context manager like:

with plt.style.context(utils.get_style("2-col")):

Alternatively you could just include them full-size in the paper. Both seem OK -- it's an appendix!

Generally easier to read if font size is consistent throughout figures and not depend on the size of the figure itself.

kellinpelrine commented 1 year ago

There's already an assert statement at the beginning (line 53) that will throw an error if wr_or_scorediff is wrong, i.e.

assert wr_or_scorediff in ['wr','scorediff','scorediff_advwin','scorediff_advlose']

Does this cover what you have in mind or still better to do it differently?

---

View entire conversation on ReviewNB

kellinpelrine commented 1 year ago

LGTM, just some minor suggested changes to make.

Sounds good, replied with question on one of them, will make the others and then merge.

AdamGleave commented 1 year ago

Ah, missed that, yeah how you have it now is fine then.

---

View entire conversation on ReviewNB

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

[AdamGleave]

Actually, one comment not on the notebook itself but on the inclusion of the figures in the paper, for the figures restricted to adversary won/lost you've used the full-size figure but downscaled it. Can you switch those to the 2col style instead then? You can do this with a context manager like:

with plt.style.context(utils.get_style("2-col")):

Alternatively you could just include them full-size in the paper. Both seem OK -- it's an appendix!

Generally easier to read if font size is consistent throughout figures and not depend on the size of the figure itself.

[kellinpelrine]

There's already an assert statement at the beginning (line 53) that will throw an error if wr_or_scorediff is wrong, i.e.

assert wr_or_scorediff in ['wr','scorediff','scorediff_advwin','scorediff_advlose']

Does this cover what you have in mind or still better to do it differently?

---

View entire conversation on ReviewNB

[kellinpelrine]

LGTM, just some minor suggested changes to make.

Sounds good, replied with question on one of them, will make the others and then merge.

[AdamGleave]

Ah, missed that, yeah how you have it now is fine then.

---

View entire conversation on ReviewNB