[codex] Sync student-facing Notebook 3 (GAMA)#10
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
bernalde
left a comment
There was a problem hiding this comment.
I reviewed the current PR head locally and ran:
/home/bernalde/repos/QuIP/.venv/bin/python -m unittest discover -s tests
~/.local/bin/uv run --group docs python -m unittest discover -s tests
QUIP_NOTEBOOK_TIMEOUT=3600 /home/bernalde/repos/QuIP/.venv/bin/python ./scripts/verify_notebooks.py notebooks_py/3-GAMA_python.ipynb notebooks_jl/3-GAMA.ipynb
make verify-julia-colab-smokes COLAB_JULIA_SMOKE_NOTEBOOKS=3-GAMA
The Python and docs unit suites passed, and the Julia Colab smoke passed. Direct execution of notebooks_py/3-GAMA_python.ipynb fails on the current head with ModuleNotFoundError: No module named 'Py4ti2int32', so I found two changes that should be made before merge and recorded them inline below.
|
Follow-up on review #4094429003: both inline findings are fixed in 4cd43c2. The Python Notebook 3 environment note now documents the local I re-ran: The three Python test runs passed. The Julia smoke finished successfully, although it still emits the existing The remaining conversation comment on this PR is the ReviewNB bot message, which is informational only and did not require a code change. |
|
View / edit / reply to this conversation on ReviewNB bernalde commented on 2026-04-17T22:26:23Z Where are the other examples? bernalde commented on 2026-04-17T23:08:12Z Still unresolved |
|
View / edit / reply to this conversation on ReviewNB bernalde commented on 2026-04-17T22:26:24Z In nbreview I can see the \n symbols in red. |
|
View / edit / reply to this conversation on ReviewNB bernalde commented on 2026-04-17T22:26:24Z Line #37. plot_augmentation(Y_feas, Y_aug, I_aug; experiment_name = "Full-basis augmentation") The plots between python and julia are not similar which makes me wonder if there are differences in the implementation. We should test one against the other. To avoid the sources of differences, we can load the initial solutions obtained via simulated annealing as a file by default and have them being computed using the DWave.jl package here as a backup (and the corresponding Python package in the other notebook). |
|
View / edit / reply to this conversation on ReviewNB bernalde commented on 2026-04-17T22:26:25Z Line #1. plot_augmentation(Y_feas, Y_paug, I_paug; experiment_name = "Partial-basis augmentation") This title does not reflect the parameters of the partial augmentation which are of interest bernalde commented on 2026-04-17T23:14:22Z The title looks ugly because it is too wide. Maybe split it in two lines would make it look nicer |
|
GitHub does not expose threaded replies for the ReviewNB mirror issue comments, so I am recording the follow-up here. For issuecomment-4271681753: clarified in ac2ee54. I did not add unrelated extra problem instances; both notebooks_py/3-GAMA_python.ipynb and notebooks_jl/3-GAMA.ipynb now state explicitly that Notebook 3 uses one shared worked instance and then compares the augmentation variants on that same instance. For issuecomment-4271681802: fixed in ac2ee54. notebooks_jl/3-GAMA.ipynb now uses actual line breaks in the example markdown instead of literal For issuecomment-4271681854: fixed in ac2ee54. Both notebooks now load the same shared coefficients, feasible starts, and Graver-direction order from For issuecomment-4271681888: fixed in ac2ee54. The partial-augmentation figure titles in both notebooks_py/3-GAMA_python.ipynb and notebooks_jl/3-GAMA.ipynb now include the sampled-direction count so the title carries the relevant experiment parameter. |
|
Still unresolved View entire conversation on ReviewNB |
|
The title looks ugly because it is too wide. Maybe split it in two lines would make it look nicer View entire conversation on ReviewNB |
|
View / edit / reply to this conversation on ReviewNB bernalde commented on 2026-04-17T23:19:56Z The users will not care about having both notebooks, we should not mention it explicitly. Let's say that we are saving the instances. Here we arbitrarily chose the Julia generation instead of the Python one. How about if we avoid this just by saving the file later when deciding whether not to load the instance |
|
View / edit / reply to this conversation on ReviewNB bernalde commented on 2026-04-17T23:19:56Z Line #57. function get_default_feasible_starts(A, b; num_reads = 20) We should first see if we load this before we commit to defining a function on how to find them |
|
View / edit / reply to this conversation on ReviewNB bernalde commented on 2026-04-17T23:19:57Z Line #68. function load_graver_order(num_directions::Int) We should first load these instead of defining how to compute them ahead of time |
|
View / edit / reply to this conversation on ReviewNB bernalde commented on 2026-04-17T23:19:57Z Are we really using the greedy way of choosing bases above? I though we were doing the complete search |
|
View / edit / reply to this conversation on ReviewNB bernalde commented on 2026-04-17T23:19:58Z Line #31. plot_augmentation_runtime(T_aug, T_paug; partial_label = "$(num_partial_directions) sampled Graver directions") Warning: No strict ticks found |
|
View / edit / reply to this conversation on ReviewNB bernalde commented on 2026-04-17T23:19:58Z Line #47. plot_multiple_partial_augmentation(Y_feas, Y_mpaug, minimum(Y_aug)) ERROR: syntax error and the y-axis label looks ugle because the tick before the zero is 0.010000000000000000002). How about if we use scientific notation and only show a single digit? |
bernalde
left a comment
There was a problem hiding this comment.
- blocking issues
- Notebook 3 reference links are broken in the rendered book because the raw
<a id=...>anchors are parsed as empty links and MyST reports missing#reference-1/#reference-2targets. - The Julia Notebook 3 final plot emits repeated
ERROR: syntax errorstderr during execution, and those errors are committed and published with the notebook output.
- nonblocking issues
- None.
- questions
- None.
- tests run and outcomes
/home/bernalde/repos/QuIP/.venv/bin/python -m unittest tests.test_notebook3_pair_sync: passed, 20 tests./home/bernalde/.local/bin/uv run --group docs python -m unittest discover -s tests: passed, 46 tests./home/bernalde/.local/bin/uv run --group docs jupyter book build --html --ci: exited 0, but reported the Notebook 3 reference-link warnings called out above.QUIP_NOTEBOOK_TIMEOUT=3600 /home/bernalde/.local/bin/uv run --group docs --group qubo python ./scripts/verify_notebooks.py notebooks_py/3-GAMA_python.ipynb: passed.make verify-julia-colab-smokes COLAB_JULIA_SMOKE_NOTEBOOKS=3-GAMA: exited 0 with3-GAMA imports ok; the local Julia environment also logged a D-Wave token warning and anIJuliaPythonCallExtextension-load error during import.QUIP_NOTEBOOK_TIMEOUT=3600 /home/bernalde/.local/bin/uv run --group docs --group qubo python ./scripts/verify_notebooks.py notebooks_jl/3-GAMA.ipynb: exited 0, but.nbverify/3-GAMA.ipynbreproduced the repeated final-cellERROR: syntax errorstderr called out above.
- whether the PR should be merged as-is
No. I would not merge this until the blocking issues above are addressed.
Note: I attempted to submit this as REQUEST_CHANGES, but GitHub rejected that state because the authenticated account is the PR author. Submitting as COMMENT with blocking inline comments is the strongest review state available from this account.
|
GitHub does not expose threaded replies for the ReviewNB mirror issue comments, so I am recording the follow-up here. For issuecomment-4271681753 and issuecomment-4271828724: addressed in a0278b8. I did not add unrelated examples; instead the notebooks now describe this as one reproducible saved instance and no longer point users at vague "other examples" from this section. For issuecomment-4271681854: addressed. The shared coefficients, feasible starts, and Graver-direction order remain loaded from For issuecomment-4271681888 and issuecomment-4271858334: addressed in a0278b8. The full and partial augmentation titles in both notebooks now include the relevant direction count and split the title across two lines. For issuecomment-4271890148: addressed in a0278b8. I removed student-facing language about "both notebooks", "directly comparable", and "Python and Julia versions begin"; the text now says the notebook loads a saved reproducible instance. For issuecomment-4271890198: addressed in a0278b8. The feasible-start loading/default helpers now appear before the fallback sampler implementation in both notebooks, and the sync test asserts that order. For issuecomment-4271890251: addressed in a0278b8. For issuecomment-4271890289: addressed in a0278b8. The titles now say "Complete-basis greedy augmentation", and the markdown clarifies that "complete basis" refers to the direction set while the selection rule remains greedy single-move, not an exhaustive complete search. For issuecomment-4271890333: addressed in a0278b8. The Julia runtime plot now uses explicit log ticks via For issuecomment-4271890400: addressed in a0278b8. The Julia sample labels now use plain text instead of math text containing Tests run after the fixes: All commands exited 0. The docs build still reports the pre-existing empty |
|
View / edit / reply to this conversation on ReviewNB bernalde commented on 2026-05-07T23:00:46Z The figure title overlaps with the line |
|
View / edit / reply to this conversation on ReviewNB bernalde commented on 2026-05-07T23:00:47Z The figure title overlaps with the line |
|
View / edit / reply to this conversation on ReviewNB bernalde commented on 2026-05-07T23:00:47Z The legend lies outside the plot bernalde commented on 2026-05-07T23:24:03Z Now the legend is overlapping the line |
|
View / edit / reply to this conversation on ReviewNB bernalde commented on 2026-05-07T23:00:48Z This sentence is weirdly written |
|
GitHub does not expose threaded replies for the ReviewNB mirror issue comments, so I am recording the follow-up here. For issuecomment-4401793537: addressed in e0db545. I shortened the complete-basis Julia plot title to a single line and added top margin in the shared For issuecomment-4401793577: addressed in e0db545. I applied the same shorter one-line title format to the partial-basis plot, keeping the sampled-direction count in the title without the overlapping two-line layout. For issuecomment-4401793620: addressed in e0db545. The Julia runtime plot now uses For issuecomment-4401793672: addressed in e0db545. I rewrote the transition after the 10-direction experiment as a speed/quality tradeoff statement before the larger-fraction sampling experiment. For issuecomment-4271681753 and issuecomment-4271828724: addressed in e0db545. I restored the class-example context by adding a Also in e0db545, I renamed the saved data files to make their scope explicit: Tests run after the fixes: All commands exited 0. The docs build still reports the existing empty |
|
Now the legend is overlapping the line View entire conversation on ReviewNB |
|
GitHub does not expose threaded replies for the ReviewNB mirror issue comments, so I am recording the follow-up here. For issuecomment-4401793620: addressed in eaf0a2d. The Julia runtime plot now uses For issuecomment-3103631606: addressed in eaf0a2d. The prior response treated Examples 1-3 as out of scope, but the latest clarification says they are class examples. I restored Examples 1, 2, and 3 from the historical Notebook 6 into both notebooks_py/3-GAMA_python.ipynb and notebooks_jl/3-GAMA.ipynb as class-reference problem definitions. The executable workflow remains on Tests run:
|
Closes #7.
Summary
This PR aligns the Python and Julia Notebook 3 materials at the student-facing level without moving logic out of the notebooks.
What Changed
Validation
/home/bernalde/repos/QuIP/.venv/bin/python -m unittest tests.test_notebook3_pair_sync/home/bernalde/repos/QuIP/.venv/bin/python -m unittest discover -s tests~/.local/bin/uv run --group docs python -m unittest discover -s tests