Skip to content

[codex] Sync student-facing Notebook 3 (GAMA)#10

Merged
bernalde merged 12 commits into
mainfrom
codex/sync-notebook3-gama
May 8, 2026
Merged

[codex] Sync student-facing Notebook 3 (GAMA)#10
bernalde merged 12 commits into
mainfrom
codex/sync-notebook3-gama

Conversation

@bernalde

Copy link
Copy Markdown
Member

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

  • aligned the Notebook 3 teaching flow around the GAMA introduction, Graver basis introduction, problem statement, example, and the feasible-starting-point QUBO section
  • added Python-side environment and execution notes near the notebook setup/import area
  • added a shared references section to the Python notebook
  • updated the Julia Notebook 3 metadata version so it matches the committed Julia environment
  • added a Notebook 3 pair-sync test module that checks headings, setup guidance, references, and Julia metadata

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

@review-notebook-app

Copy link
Copy Markdown

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@bernalde bernalde left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread notebooks_py/3-GAMA_python.ipynb Outdated
Comment thread tests/test_notebook3_pair_sync.py

Copy link
Copy Markdown
Member Author

Follow-up on review #4094429003: both inline findings are fixed in 4cd43c2.

The Python Notebook 3 environment note now documents the local 4ti2 / Py4ti2 / Py4ti2int32 requirement in notebooks_py/3-GAMA_python.ipynb, and the pair-sync regression test now enforces that note in tests/test_notebook3_pair_sync.py.

I re-ran:
/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
make verify-julia-colab-smokes COLAB_JULIA_SMOKE_NOTEBOOKS=3-GAMA
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

The three Python test runs passed. The Julia smoke finished successfully, although it still emits the existing IJulia extension warnings in this environment. Direct local execution of notebooks_py/3-GAMA_python.ipynb still fails here on the external Py4ti2int32 dependency, which is why the note now calls that requirement out explicitly.

The remaining conversation comment on this PR is the ReviewNB bot message, which is informational only and did not require a code change.

@bernalde bernalde marked this pull request as ready for review April 12, 2026 02:47
Comment thread notebooks_jl/3-GAMA.ipynb
Comment thread notebooks_jl/3-GAMA.ipynb
Comment thread notebooks_jl/3-GAMA.ipynb
Comment thread notebooks_jl/3-GAMA.ipynb
Comment thread notebooks_jl/3-GAMA.ipynb
Comment thread notebooks_py/3-GAMA_python.ipynb
Comment thread notebooks_py/3-GAMA_python.ipynb
Comment thread notebooks_py/3-GAMA_python.ipynb
Comment thread notebooks_py/3-GAMA_python.ipynb
Comment thread notebooks_py/3-GAMA_python.ipynb
Comment thread notebooks_jl/3-GAMA.ipynb
Comment thread notebooks_jl/3-GAMA.ipynb
Comment thread notebooks_jl/3-GAMA.ipynb
Comment thread notebooks_jl/3-GAMA.ipynb
Comment thread notebooks_jl/3-GAMA.ipynb
Comment thread notebooks_py/3-GAMA_python.ipynb
Comment thread notebooks_py/3-GAMA_python.ipynb
Comment thread notebooks_py/3-GAMA_python.ipynb
Comment thread notebooks_py/3-GAMA_python.ipynb
Comment thread notebooks_py/3-GAMA_python.ipynb
@review-notebook-app

review-notebook-app Bot commented Apr 17, 2026

Copy link
Copy Markdown

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

@review-notebook-app

review-notebook-app Bot commented Apr 17, 2026

Copy link
Copy Markdown

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.


@review-notebook-app

review-notebook-app Bot commented Apr 17, 2026

Copy link
Copy Markdown

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).


@review-notebook-app

review-notebook-app Bot commented Apr 17, 2026

Copy link
Copy Markdown

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

Comment thread notebooks_py/3-GAMA_python.ipynb
Comment thread notebooks_py/3-GAMA_python.ipynb
Comment thread notebooks_py/3-GAMA_python.ipynb
Comment thread notebooks_py/3-GAMA_python.ipynb

Copy link
Copy Markdown
Member Author

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 \n sequences, and tests/test_notebook3_pair_sync.py now checks that those literal sequences do not return.

For issuecomment-4271681854: fixed in ac2ee54. Both notebooks now load the same shared coefficients, feasible starts, and Graver-direction order from notebooks_data, with the local samplers used only as fallback paths, and tests/test_notebook3_pair_sync.py now checks that shared-data path and the matching objective sign convention. This is also the commit where both notebooks were rerun before pushing.

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.

Copy link
Copy Markdown
Member Author

Still unresolved


View entire conversation on ReviewNB

Copy link
Copy Markdown
Member Author

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

@review-notebook-app

review-notebook-app Bot commented Apr 17, 2026

Copy link
Copy Markdown

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


@review-notebook-app

review-notebook-app Bot commented Apr 17, 2026

Copy link
Copy Markdown

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


@review-notebook-app

review-notebook-app Bot commented Apr 17, 2026

Copy link
Copy Markdown

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


@review-notebook-app

review-notebook-app Bot commented Apr 17, 2026

Copy link
Copy Markdown

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


@review-notebook-app

review-notebook-app Bot commented Apr 17, 2026

Copy link
Copy Markdown

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


@review-notebook-app

review-notebook-app Bot commented Apr 17, 2026

Copy link
Copy Markdown

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 bernalde left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 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-2 targets.
  • The Julia Notebook 3 final plot emits repeated ERROR: syntax error stderr during execution, and those errors are committed and published with the notebook output.
  1. nonblocking issues
  • None.
  1. questions
  • None.
  1. 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 with 3-GAMA imports ok; the local Julia environment also logged a D-Wave token warning and an IJuliaPythonCallExt extension-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.ipynb reproduced the repeated final-cell ERROR: syntax error stderr called out above.
  1. 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.

Comment thread tests/test_notebook3_pair_sync.py Outdated
Comment thread tests/test_notebook3_pair_sync.py Outdated
@bernalde

bernalde commented May 7, 2026

Copy link
Copy Markdown
Member Author

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 notebooks_data by default, with sampler computation only as fallback. a0278b8 keeps that path, refreshes both executed notebooks, and the sync test continues to cover the shared-data behavior.

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. load_graver_order now presents the saved-data path first, with deterministic permutation generation only as the fallback path; the sync test asserts the load helper appears before get_feasible.

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 log_ticks_for, and the committed output was refreshed without No strict ticks found.

For issuecomment-4271890400: addressed in a0278b8. The Julia sample labels now use plain text instead of math text containing %, objective-gap ticks use one-digit scientific notation, and the committed output was refreshed without ERROR: syntax error.

Tests run after the fixes:
env QUIP_NOTEBOOK_TIMEOUT=3600 ~/.local/bin/uv run --group docs --group qubo python ./scripts/verify_notebooks.py notebooks_jl/3-GAMA.ipynb
env QUIP_NOTEBOOK_TIMEOUT=3600 ~/.local/bin/uv run --group docs --group qubo python ./scripts/verify_notebooks.py notebooks_py/3-GAMA_python.ipynb
.venv/bin/python -m unittest tests.test_notebook3_pair_sync
~/.local/bin/uv run --group docs python -m unittest discover -s tests
~/.local/bin/uv run --group docs jupyter book build --html --ci
make verify-julia-colab-smokes COLAB_JULIA_SMOKE_NOTEBOOKS=3-GAMA

All commands exited 0. The docs build still reports the pre-existing empty #top link warnings in older Python notebooks, and the Julia smoke still reports the existing local D-Wave token / IJulia extension warnings before 3-GAMA imports ok; neither is introduced by this fix.

@review-notebook-app

review-notebook-app Bot commented May 7, 2026

Copy link
Copy Markdown

View / edit / reply to this conversation on ReviewNB

bernalde commented on 2026-05-07T23:00:46Z
----------------------------------------------------------------

The figure title overlaps with the line


@review-notebook-app

review-notebook-app Bot commented May 7, 2026

Copy link
Copy Markdown

View / edit / reply to this conversation on ReviewNB

bernalde commented on 2026-05-07T23:00:47Z
----------------------------------------------------------------

The figure title overlaps with the line


@review-notebook-app

review-notebook-app Bot commented May 7, 2026

Copy link
Copy Markdown

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

@review-notebook-app

review-notebook-app Bot commented May 7, 2026

Copy link
Copy Markdown

View / edit / reply to this conversation on ReviewNB

bernalde commented on 2026-05-07T23:00:48Z
----------------------------------------------------------------

This sentence is weirdly written


@bernalde

bernalde commented May 7, 2026

Copy link
Copy Markdown
Member Author

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 plot_augmentation helper so the title no longer overlaps the plotted line.

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 legend = :topright instead of a coordinate legend placement that could put it outside the plot.

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 Class examples section in both Notebook 3 variants that points students to the QUBO and Benchmarking class examples, while making clear that the remaining GAMA workflow uses the saved Example 4 portfolio instance.

Also in e0db545, I renamed the saved data files to make their scope explicit:
notebooks_data/3-GAMA_example4_coefficients.csv
notebooks_data/3-GAMA_example4_feasible_starts.csv
notebooks_data/3-GAMA_example4_graver_order.csv

Tests run after the fixes:
env QUIP_NOTEBOOK_TIMEOUT=3600 ~/.local/bin/uv run --group docs --group qubo python ./scripts/verify_notebooks.py notebooks_jl/3-GAMA.ipynb
env QUIP_NOTEBOOK_TIMEOUT=3600 ~/.local/bin/uv run --group docs --group qubo python ./scripts/verify_notebooks.py notebooks_py/3-GAMA_python.ipynb
.venv/bin/python -m unittest tests.test_notebook3_pair_sync
~/.local/bin/uv run --group docs python -m unittest discover -s tests
~/.local/bin/uv run --group docs jupyter book build --html --ci
make verify-julia-colab-smokes COLAB_JULIA_SMOKE_NOTEBOOKS=3-GAMA

All commands exited 0. The docs build still reports the existing empty #top link warnings in Python notebooks, and the Julia smoke still reports the existing local D-Wave token / IJulia extension warnings before 3-GAMA imports ok; neither is introduced by this change.

bernalde commented May 7, 2026

Copy link
Copy Markdown
Member Author

Now the legend is overlapping the line


View entire conversation on ReviewNB

@bernalde

bernalde commented May 7, 2026

Copy link
Copy Markdown
Member Author

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 legend = :outertopright with an explicit right margin in notebooks_jl/3-GAMA.ipynb, and I re-executed the notebook so the committed output shows the legend outside the plot area rather than overlapping either line. tests/test_notebook3_pair_sync.py now rejects the old :topright placement.

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 ### Example 4: saved portfolio instance, and the 3-GAMA_example4_* data files remain named for that saved instance. The sync test now checks the restored example data.

Tests run:

  • .venv/bin/python -m unittest tests.test_notebook3_pair_sync: passed.
  • env QUIP_NOTEBOOK_TIMEOUT=3600 ~/.local/bin/uv run --group docs --group qubo python ./scripts/verify_notebooks.py notebooks_py/3-GAMA_python.ipynb: passed.
  • env QUIP_NOTEBOOK_TIMEOUT=3600 ~/.local/bin/uv run --group docs --group qubo python ./scripts/verify_notebooks.py notebooks_jl/3-GAMA.ipynb: passed.
  • ~/.local/bin/uv run --group docs python -m unittest discover -s tests: passed, 47 tests.
  • ~/.local/bin/uv run --group docs jupyter book build --html --ci: passed; it still reports the pre-existing empty #top link warnings on several Python notebooks.
  • make verify-julia-colab-smokes COLAB_JULIA_SMOKE_NOTEBOOKS=3-GAMA: passed; it still prints the existing DWAVE_API_TOKEN warning and IJulia extension loading warning before 3-GAMA imports ok.

@bernalde bernalde merged commit d5a37e7 into main May 8, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sync Python and Julia student-facing content for Notebook 3 (GAMA)

1 participant