Skip to content

PML-313: commits cleanup and conflict resolution#273

Open
leithkarrai wants to merge 12 commits into
merlinquantum:release/0.4.1from
leithkarrai:PML-313-v2
Open

PML-313: commits cleanup and conflict resolution#273
leithkarrai wants to merge 12 commits into
merlinquantum:release/0.4.1from
leithkarrai:PML-313-v2

Conversation

@leithkarrai

Copy link
Copy Markdown

Summary

Related Issue

Type of change

  • Bug fix
  • New feature
  • Documentation update
  • Refactor / Cleanup
  • Performance improvement
  • CI / Build / Tooling
  • Breaking change (requires migration notes)

Proposed changes

How to test / How to run

  1. Command lines
Block of code

Screenshots / Logs (optional)

Performance considerations (optional)

Documentation

  • User docs updated (Sphinx)
  • Examples / notebooks updated
  • Docstrings updated
  • Updated the API

Checklist

  • [x ] PR title includes Jira issue key (e.g., PML-126)

  • [ x] "Related Jira ticket" section includes the Jira issue key (no URL)

  • [x ] Code formatted (ruff format)

  • [x ] Lint passes (ruff)

  • [ x] Static typing passes (mypy) if applicable

  • [ x] Unit tests added/updated (pytest)

  • [ x] Tests pass locally (pytest)

  • Tests pass on GPU (pytest)

  • Test coverage not decreased significantly

  • [ x] Docs build locally if affected (sphinx)

  • With this command:

      > SPHINXOPTS="-W --keep-going -n" make -C docs clean html 
    

    the docs are built without any warning or errors.

  • New public classes/methods/packages are added in the API following the methodology presented in other files.

  • Dependencies updated (if needed) and pinned appropriately

  • [ x] PR description explains what changed and how to validate it

@LF-Vigneux LF-Vigneux self-assigned this Jun 19, 2026

@LF-Vigneux LF-Vigneux left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some more changes, one needs ti be confirmed by Cassandre

Comment thread benchmarks/QCNN_scaling_study_benchmark.py Outdated
Comment thread benchmarks/scaling_study_graphs.py Outdated
Comment thread benchmarks/scaling_study_graphs.py
Comment thread docs/source/_static/img/graph_scaling_study.png
Comment thread docs/source/quantum_expert_area/scaling_study_of_qcnn.rst Outdated
Comment thread docs/source/quantum_expert_area/scaling_study_of_qcnn.rst Outdated
Comment thread merlin/models/qcnn.py Outdated
Comment thread pytest.ini Outdated
@LF-Vigneux

Copy link
Copy Markdown
Contributor

@claude review

@claude

claude Bot commented Jun 19, 2026

Copy link
Copy Markdown

Claude finished @LF-Vigneux's task in 1m 33s —— View job


Claude PR Review

  • Read CLAUDE.md & gather context
  • Review benchmark scripts
  • Review docs (rst, json)
  • Review qcnn.py changes & tests
  • Review pytest.ini change
  • Post review feedback

This PR adds a QCNN scaling study: a benchmark script, a plotting script, a generated graph + gallery entry, an expert-docs page, and a UserWarning guard on large input shapes. The direction is good and the docs page is a nice addition. Below is my independent review — it overlaps with and reinforces @LF-Vigneux's inline comments.


🔴 Blocking / correctness

1. The three CSV paths don't agree — the documented pipeline won't reproduce.

  • QCNN_scaling_study_benchmark.py:9-11 writes to ./docs/source/quantum_expert_area/input_shape_logs/scaling_study_benchmark.csv
  • scaling_study_graphs.py:18 reads scaling_study_benchmark.csv (relative to the current working dir)
  • The committed CSV actually lives at benchmarks/scaling_study_benchmark.csv

So if a user follows the doc and runs both scripts from the repo root, the graph script won't find the file the benchmark just wrote. Pick one canonical location and use it in all three places.

2. scaling_study_of_qcnn.rst:48-49 documents a module path that doesn't exist.

python -m docs.source.quantum_expert_area.input_shape_logs.scaling_study_graphs

The plotting script is at benchmarks/scaling_study_graphs.py, so this -m path will fail. Both commands should reference the real locations (python -m benchmarks.scaling_study_graphs, assuming the path issue in #1 is resolved).

3. Threshold vs. documented cap are off by one — 24 both "safe" and "warned".
qcnn.py:145 warns when input_shape[0] * input_shape[1] >= 576, i.e. 24×24 = 576 triggers the warning. But scaling_study_of_qcnn.rst:27 says the input dimension is "strictly capped at 24" as the safe value. So the documented safe maximum is exactly the value that fires the warning. Either make the doc say "below 24" / "up to 23", or change the guard to > 576. Also, please replace the magic 576 with a named constant or an inline comment (# 24 * 24) so the intent is discoverable.


🟡 Reinforcing the existing review comments

  • pytest.ini:31 — Agree with @LF-Vigneux. Adding a global ignore:...Too many dimensions...:UserWarning is the wrong scope: this isn't a deprecation, and it suppresses the warning for every test, not just the one asserting it. test_qcnn_input_shape_warning already captures it via pytest.warns(...), so the global filter is unnecessary. If another test legitimately builds a large QCNN, scope the suppression there with @pytest.mark.filterwarnings(...) or warnings.catch_warnings() instead.
  • qcnn.py:145 — Agree this needs the v4.0 confirmation from Cassandre before landing. If the unrestricted version is out, the guard may be obsolete.
  • Typos / language (matching the inline notes): ÉCHECError (QCNN_scaling_study_benchmark.py:71), datasdata and wcnnqcnn (scaling_study_graphs.py:2,4), Tendance/Tendanytendency, Paramètres/Paramètres réelsParameters, tendanytendency in the graph image, and empirical complexity lawsempirical complexities + "merlin is not an environment" (scaling_study_of_qcnn.rst:5,43).
  • Per-graph comments in scaling_study_graphs.py — agree, a one-line comment above each subplot block (what axis/metric it shows) would help.

🟢 Additional smaller findings

  • QCNN_scaling_study_benchmark.py:31"Begining""Beginning".
  • peak_memory_mb is not a peak. Both benchmark.py:57-58 and the CSV column record a single process.memory_info().rss snapshot, not a peak. Either rename the column to rss_memory_mb or use resource.getrusage/tracemalloc to capture an actual peak — otherwise the graph's "RAM consumption" axis is mislabeled.
  • psutil is an undeclared dependency. It's imported in the benchmark but isn't in pyproject.toml. Even for a benchmark script, consider adding it under a dev/benchmark extra so the pipeline is reproducible.
  • scaling_study_graphs.py:2-9 docstring is inaccurate — it lists "RAM, memory footprint, number of parameters" but the script plots runtime, parameters, and RAM (6 graphs). The unused import os (line 15) can also be dropped.
  • tests/models/test_qcnn.py — missing newline at end of file, and only one blank line before def test_qcnn_input_shape_warning (PEP8/ruff want two). index.rst is also missing a trailing newline. Running ruff format should fix these.
  • Benchmark robustness — the bare except Exception is acceptable for a throwaway benchmark, but the break only fires when batch_size == phase2_batch; a failure during phase 1 silently continues, which may not be intended.

Suggested fixes

Fix CSV path mismatch + doc module path →

Fix threshold/doc off-by-one + magic number →

I did not make any code changes (this was a review request). Happy to implement any of the above if you'd like — just say the word.
· branch PML-313-v2

Comment thread docs/source/performance/scaling_study_of_qcnn.rst
Comment thread benchmarks/scaling_study_benchmark.csv Outdated
@leithkarrai leithkarrai changed the base branch from release/0.4 to release/0.4.1 June 23, 2026 14:10

@LF-Vigneux LF-Vigneux left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Last changes

Comment thread pyproject.toml Outdated

@LF-Vigneux LF-Vigneux left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good, update the branch and I merge. A couple of remarks on teams to check first

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.

2 participants