Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
bernalde
left a comment
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. |
| "attachments": {}, | ||
| "cell_type": "markdown", | ||
| "metadata": {}, | ||
| "source": [ |
There was a problem hiding this comment.
Can't the colab notebook by importing the right Julia package compute the Graver basis?
Reply via ReviewNB
There was a problem hiding this comment.
Fixed in 29d2a2d. notebooks_jl/3-GAMA.ipynb now states explicitly that the notebook uses the lib4ti2_jll graver binary when it is available in Colab or locally, and only falls back to the bundled graver.npy file when that binary is unavailable. The fallback reader in the same file was also corrected to use NPZ.npyread, so both paths are now consistent.
| "attachments": {}, | ||
| "cell_type": "markdown", | ||
| "metadata": {}, | ||
| "source": [ |
There was a problem hiding this comment.
Line #56. function compute_graver_basis_local(A)
Doesn't this function require the bounds on the variables?
Reply via ReviewNB
There was a problem hiding this comment.
Clarified in 29d2a2d. The docstring on notebooks_jl/3-GAMA.ipynb now explains that compute_graver_basis_local(A) only writes the constraint matrix because the Graver basis depends on A alone; the box bounds are enforced later during augmentation. The regression check in tests/test_notebook3_pair_sync.py still verifies that no .lb or .ub files are written.
| "attachments": {}, | ||
| "cell_type": "markdown", | ||
| "metadata": {}, | ||
| "source": [ |
There was a problem hiding this comment.
Line #2. function bisection_rule(f::Function, g, x, xl = nothing, xu = nothing, laststep = nothing)
It would be useful for this and the other functions in this notebook to provide documentation
Reply via ReviewNB
There was a problem hiding this comment.
Fixed in 29d2a2d. I added docstrings across the notebook helpers in notebooks_jl/3-GAMA.ipynb, including the Graver-basis routines, step rules, augmentation routines, experiment helpers, and plotting helpers. tests/test_notebook3_pair_sync.py now checks for the new documentation anchors.
| "attachments": {}, | ||
| "cell_type": "markdown", | ||
| "metadata": {}, | ||
| "source": [ |
There was a problem hiding this comment.
It is unclear why not all the initial points with the complete augmentation not reach the same (global) minimum
Reply via ReviewNB
There was a problem hiding this comment.
Fixed in 29d2a2d. The narrative in notebooks_jl/3-GAMA.ipynb now explains that this notebook uses the greedy single-move augmentation rule on a nonseparable convex objective, so different feasible starts can end at different local optima even with the full Graver basis.
| "attachments": {}, | ||
| "cell_type": "markdown", | ||
| "metadata": {}, | ||
| "source": [ |
There was a problem hiding this comment.
Line #1. plot_augmentation(Y_feas, Y_paug, I_paug)
This function should receive the name of the experiment to set a title to the figures
Reply via ReviewNB
There was a problem hiding this comment.
Fixed in 29d2a2d. notebooks_jl/3-GAMA.ipynb now gives plot_augmentation an experiment_name argument and uses it in the full-basis and partial-basis calls, so the figures carry the experiment context directly. tests/test_notebook3_pair_sync.py now checks those labels.
| @@ -38,7 +38,11 @@ | |||
| "cell_type": "markdown", | |||
There was a problem hiding this comment.
Line #2. def single_move(g: np.ndarray, fun: Callable, x: np.ndarray, x_lo: np.ndarray = None, x_up: np.ndarray = None, laststep: np.ndarray = None) -> (float, int):
This function should be moved to a place that makes more sense within the code
Reply via ReviewNB
There was a problem hiding this comment.
Fixed in 29d2a2d. I moved the augmentation helper cells in notebooks_py/3-GAMA_python.ipynb so single_move, bisection, and the related selection helpers now sit with the objective, constraint, and augmentation code instead of being stranded earlier in the notebook.
| @@ -38,7 +38,11 @@ | |||
| "cell_type": "markdown", | |||
There was a problem hiding this comment.
Line #1. def augmentation(grav = r, func = f, x = x0, x_lo = x_lo, x_up = x_up, OPTION: int = 1, VERBOSE: bool = True, itermax: int = 1000) -> (int, float, np.ndarray):
Let's add docs to this and all other functions
Reply via ReviewNB
There was a problem hiding this comment.
Fixed in 29d2a2d. I added docstrings across the Python notebook helpers in notebooks_py/3-GAMA_python.ipynb, including the candidate selection helpers, the step rules, the objective and constraint helpers, the Graver-basis loader, get_feasible, and augmentation. tests/test_notebook3_pair_sync.py now checks the documentation anchors.
| @@ -38,7 +38,11 @@ | |||
| "cell_type": "markdown", | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
Fixed in 29d2a2d. notebooks_py/3-GAMA_python.ipynb now includes the actual QUBO formulation for the feasible-start problem, including the \|Ax - b\|_2^2 objective, its quadratic expansion, and how that maps to the Q and offset values used in code. tests/test_notebook3_pair_sync.py now checks that explanation.
| @@ -38,7 +38,11 @@ | |||
| "cell_type": "markdown", | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
Fixed in 29d2a2d. notebooks_py/3-GAMA_python.ipynb now explains why simulated annealing is used here: the goal is a diverse set of feasible starting points, while a MIP solver would usually stop after one optimum instead of sampling several distinct zero-energy states of the same QUBO.
| @@ -38,7 +38,11 @@ | |||
| "cell_type": "markdown", | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
Partially addressed in 29d2a2d. notebooks_py/3-GAMA_python.ipynb no longer stops at the local Py4ti2int32 import; it now falls back to the bundled graver.npy file so the later cells can run even without a separate local 4ti2 install. I did not commit fresh executed outputs in this change, but the source-level blocker that was preventing the rest of the notebook from running locally is now removed, and tests/test_notebook3_pair_sync.py covers the fallback path.
| "attachments": {}, | ||
| "cell_type": "markdown", | ||
| "metadata": {}, | ||
| "source": [ |
There was a problem hiding this comment.
Can't the colab notebook by importing the right Julia package compute the Graver basis?
Reply via ReviewNB
There was a problem hiding this comment.
Fixed in 29d2a2d. notebooks_jl/3-GAMA.ipynb now states explicitly that the notebook uses the lib4ti2_jll graver binary when it is available in Colab or locally, and only falls back to the bundled graver.npy file when that binary is unavailable. The fallback reader in the same file was also corrected to use NPZ.npyread, so both paths are now consistent.
| "attachments": {}, | ||
| "cell_type": "markdown", | ||
| "metadata": {}, | ||
| "source": [ |
There was a problem hiding this comment.
Line #56. function compute_graver_basis_local(A)
Doesn't this function require the bounds on the variables?
Reply via ReviewNB
There was a problem hiding this comment.
Clarified in 29d2a2d. The docstring on notebooks_jl/3-GAMA.ipynb now explains that compute_graver_basis_local(A) only writes the constraint matrix because the Graver basis depends on A alone; the box bounds are enforced later during augmentation. The regression check in tests/test_notebook3_pair_sync.py still verifies that no .lb or .ub files are written.
| "attachments": {}, | ||
| "cell_type": "markdown", | ||
| "metadata": {}, | ||
| "source": [ |
There was a problem hiding this comment.
Line #2. function bisection_rule(f::Function, g, x, xl = nothing, xu = nothing, laststep = nothing)
It would be useful for this and the other functions in this notebook to provide documentation
Reply via ReviewNB
There was a problem hiding this comment.
Fixed in 29d2a2d. I added docstrings across the notebook helpers in notebooks_jl/3-GAMA.ipynb, including the Graver-basis routines, step rules, augmentation routines, experiment helpers, and plotting helpers. tests/test_notebook3_pair_sync.py now checks for the new documentation anchors.
| "attachments": {}, | ||
| "cell_type": "markdown", | ||
| "metadata": {}, | ||
| "source": [ |
There was a problem hiding this comment.
It is unclear why not all the initial points with the complete augmentation not reach the same (global) minimum
Reply via ReviewNB
There was a problem hiding this comment.
Fixed in 29d2a2d. The narrative in notebooks_jl/3-GAMA.ipynb now explains that this notebook uses the greedy single-move augmentation rule on a nonseparable convex objective, so different feasible starts can end at different local optima even with the full Graver basis.
| "attachments": {}, | ||
| "cell_type": "markdown", | ||
| "metadata": {}, | ||
| "source": [ |
There was a problem hiding this comment.
Line #1. plot_augmentation(Y_feas, Y_paug, I_paug)
This function should receive the name of the experiment to set a title to the figures
Reply via ReviewNB
There was a problem hiding this comment.
Fixed in 29d2a2d. notebooks_jl/3-GAMA.ipynb now gives plot_augmentation an experiment_name argument and uses it in the full-basis and partial-basis calls, so the figures carry the experiment context directly. tests/test_notebook3_pair_sync.py now checks those labels.
| @@ -38,7 +38,11 @@ | |||
| "cell_type": "markdown", | |||
There was a problem hiding this comment.
Line #2. def single_move(g: np.ndarray, fun: Callable, x: np.ndarray, x_lo: np.ndarray = None, x_up: np.ndarray = None, laststep: np.ndarray = None) -> (float, int):
This function should be moved to a place that makes more sense within the code
Reply via ReviewNB
There was a problem hiding this comment.
Fixed in 29d2a2d. I moved the augmentation helper cells in notebooks_py/3-GAMA_python.ipynb so single_move, bisection, and the related selection helpers now sit with the objective, constraint, and augmentation code instead of being stranded earlier in the notebook.
| @@ -38,7 +38,11 @@ | |||
| "cell_type": "markdown", | |||
There was a problem hiding this comment.
Line #1. def augmentation(grav = r, func = f, x = x0, x_lo = x_lo, x_up = x_up, OPTION: int = 1, VERBOSE: bool = True, itermax: int = 1000) -> (int, float, np.ndarray):
Let's add docs to this and all other functions
Reply via ReviewNB
There was a problem hiding this comment.
Fixed in 29d2a2d. I added docstrings across the Python notebook helpers in notebooks_py/3-GAMA_python.ipynb, including the candidate selection helpers, the step rules, the objective and constraint helpers, the Graver-basis loader, get_feasible, and augmentation. tests/test_notebook3_pair_sync.py now checks the documentation anchors.
| @@ -38,7 +38,11 @@ | |||
| "cell_type": "markdown", | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
Fixed in 29d2a2d. notebooks_py/3-GAMA_python.ipynb now includes the actual QUBO formulation for the feasible-start problem, including the \|Ax - b\|_2^2 objective, its quadratic expansion, and how that maps to the Q and offset values used in code. tests/test_notebook3_pair_sync.py now checks that explanation.
| @@ -38,7 +38,11 @@ | |||
| "cell_type": "markdown", | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
Fixed in 29d2a2d. notebooks_py/3-GAMA_python.ipynb now explains why simulated annealing is used here: the goal is a diverse set of feasible starting points, while a MIP solver would usually stop after one optimum instead of sampling several distinct zero-energy states of the same QUBO.
| @@ -38,7 +38,11 @@ | |||
| "cell_type": "markdown", | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
Partially addressed in 29d2a2d. notebooks_py/3-GAMA_python.ipynb no longer stops at the local Py4ti2int32 import; it now falls back to the bundled graver.npy file so the later cells can run even without a separate local 4ti2 install. I did not commit fresh executed outputs in this change, but the source-level blocker that was preventing the rest of the notebook from running locally is now removed, and tests/test_notebook3_pair_sync.py covers the fallback path.
|
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 |
| @@ -29,7 +29,7 @@ | |||
| " <img src=\"https://colab.research.google.com/assets/colab-badge.svg\" alt=\"Open In Colab\"/>\n", | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
Clarified in ac2ee54. I did not add unrelated extra problem instances; instead both notebooks_py/3-GAMA_python.ipynb and notebooks_jl/3-GAMA.ipynb now state explicitly that Notebook 3 studies one shared worked instance and then compares full-basis, partial-basis, and multi-partial augmentation on that same instance so the Python and Julia results stay directly comparable. Both notebooks were rerun in this commit.
| @@ -29,7 +29,7 @@ | |||
| " <img src=\"https://colab.research.google.com/assets/colab-badge.svg\" alt=\"Open In Colab\"/>\n", | |||
There was a problem hiding this comment.
Line #2. """Apply one of the notebook's Graver augmentation strategies until convergence."""
We need better documentation explaining the arguments to the functions and what they return.
Reply via ReviewNB
There was a problem hiding this comment.
Fixed in ac2ee54. The helper functions in both notebooks_py/3-GAMA_python.ipynb and notebooks_jl/3-GAMA.ipynb now have explicit argument and return-value documentation for the augmentation, feasible-start, Graver-basis, experiment, and plotting helpers. tests/test_notebook3_pair_sync.py was extended so those richer docstrings are checked in both notebooks.
| @@ -29,7 +29,7 @@ | |||
| " <img src=\"https://colab.research.google.com/assets/colab-badge.svg\" alt=\"Open In Colab\"/>\n", | |||
There was a problem hiding this comment.
Line #8. plt.show()
A log axis for time would make the most sense here. ALso in the julia notebook
Reply via ReviewNB
There was a problem hiding this comment.
Fixed in ac2ee54. The runtime plots now use a log y-axis in both notebooks_py/3-GAMA_python.ipynb and notebooks_jl/3-GAMA.ipynb. tests/test_notebook3_pair_sync.py now checks the log-scale plotting path.
| @@ -29,7 +29,7 @@ | |||
| " <img src=\"https://colab.research.google.com/assets/colab-badge.svg\" alt=\"Open In Colab\"/>\n", | |||
There was a problem hiding this comment.
Line #12. plt.show()
The objective value is fundamentally different than in the Julia notebook. We need to review this to make sure we are solving the same problem
Reply via ReviewNB
There was a problem hiding this comment.
Fixed in ac2ee54. The mismatch came from real cross-notebook drift: the Julia notebook was using the opposite sign on the linear objective term, and the two notebooks were also sampling different starts and Graver-direction subsets. Both notebooks_py/3-GAMA_python.ipynb and notebooks_jl/3-GAMA.ipynb now load the same coefficients, feasible starts, and Graver-direction order from the shared notebooks_data files by default, and tests/test_notebook3_pair_sync.py now checks both the shared-data path and the matching objective sign convention.
|
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? |
1 participant
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