Closed tomtseng closed 1 year ago
Check out this pull request on
See visual diffs & provide feedback on Jupyter Notebooks.
Powered by ReviewNB
Name | Link |
---|---|
Latest commit | 24074183a7e318139d683ca6fc57c92c09262f29 |
Latest deploy log | https://app.netlify.com/sites/goattack/deploys/637bdc242aac050008b8a5b2 |
View / edit / reply to this conversation on ReviewNB
UFO-101 commented on 2022-11-21T17:37:15Z ----------------------------------------------------------------
Line #8. figsize=(2.64049, 2),
What's the significance of this number?
(The significance of 5.50107 used below is that it's the width of the page.)
Not sure, this was copied from existing code. It's 48% of 5.501 so I guess it's just half the width of the page minus a small margin
View / edit / reply to this conversation on ReviewNB
UFO-101 commented on 2022-11-21T17:37:16Z ----------------------------------------------------------------
Line #10. gridspec_kw={"height_ratios": [10]},
height_rations with a list of length one doesn't do anything. This is my fault cause I used it in other charts, but it doesn't need to be here.
thanks, will remove
View / edit / reply to this conversation on ReviewNB
UFO-101 commented on 2022-11-21T17:37:17Z ----------------------------------------------------------------
Line #15. * df.query(f"adv_name.str.contains('ov1') == False & victim_visits <= 128")
Should this be an f string?
thanks, will remove
View / edit / reply to this conversation on ReviewNB
UFO-101 commented on 2022-11-21T17:37:18Z ----------------------------------------------------------------
Line #25. plt.ylabel("Adv-$\\texttt{cp127}$ win \\%")
This format seems weird. Why not a space between 'Adv' and 'cp127'?
This is the plot for "what happens if we train an adversary solely on cp127 and then evaluate it against cp505?" I'll rename it the y-axis label to "cp127-trained adv. win %"
View / edit / reply to this conversation on ReviewNB
UFO-101 commented on 2022-11-21T17:37:18Z ----------------------------------------------------------------
Line #33. figsize=(2.64049, 2),
This number again
View / edit / reply to this conversation on ReviewNB
UFO-101 commented on 2022-11-21T17:37:19Z ----------------------------------------------------------------
Line #8. plot_adv_visit_sweep(df, df2, 64, "$\\texttt{Latest}$", "adv-vs-cp505-vary-visits2")
Should this plot be committed?
yeah, this is the existing plot for Figure 8b and is already committed
View / edit / reply to this conversation on ReviewNB
UFO-101 commented on 2022-11-21T17:37:20Z ----------------------------------------------------------------
Line #9. gridspec_kw={"height_ratios": [10]},
Height ratios
View / edit / reply to this conversation on ReviewNB
UFO-101 commented on 2022-11-21T17:37:21Z ----------------------------------------------------------------
Line #62. labels[i] += " ($\\texttt{Original}$)"
This loop is very similar to one above. Could it be extracted to a function?
There's definitely code duplication between this cell and the previous cell. I didn't spend time refactoring it because
View / edit / reply to this conversation on ReviewNB
UFO-101 commented on 2022-11-21T17:37:22Z ----------------------------------------------------------------
Line #63. plt.subplot(1, 1, 1)
This looks like it shouldn't be there
will remove
View / edit / reply to this conversation on ReviewNB
UFO-101 commented on 2022-11-21T17:37:23Z ----------------------------------------------------------------
Line #79. method="beta",
Maybe add a comment linking to documentation explaining what this means:
https://www.statsmodels.org/dev/generated/statsmodels.stats.proportion.proportion_confint.html
will do
Thanks for these refactors. This code definitely looks much better.
Not sure, this was copied from existing code. It's 48% of 5.501 so I guess it's just half the width of the page minus a small margin
View entire conversation on ReviewNB
This is the plot for "what happens if we train an adversary solely on cp127 and then evaluate it against cp505?" I'll rename it the y-axis label to "cp127-trained adv. win %"
View entire conversation on ReviewNB
yeah, this is the existing plot for Figure 8b and is already committed
View entire conversation on ReviewNB
There's definitely code duplication between this cell and the previous cell. I didn't spend time refactoring it because
s349284096
from the hardened training run) and the second version (s497721856
)