PML-282 Photonic Generator#220
Conversation
LF-Vigneux
left a comment
There was a problem hiding this comment.
The form and structure of the code seems fine to me. This is not a complete review pending the remaining functions to implement.
CassNot
left a comment
There was a problem hiding this comment.
There are some discrepancy with the orignal code
- (orignal perceval) https://github.com/Quandela/photonic-qgan
- (new MerLin) https://github.com/merlinquantum/reproduced_papers/tree/main/papers/photonic_QGAN
I am describing these discrepancies in review comments
More of a product question: What happens if the subgenerators are all the same ? Do we need to create multiple times the same one or should there be a shortcut to have to only define it once, then through the API, we precise the circuit/layer structure and N ones would be initiated ? (as in the original one)
Lack of validation: to validate this implementation, I would test it in the same settings as the original work (implementation here https://github.com/merlinquantum/reproduced_papers/tree/main/papers/photonic_QGAN) in a dedicated notebook
|
@codex review with respect to original implementation https://github.com/merlinquantum/reproduced_papers/tree/main/papers/photonic_QGAN |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c529552be
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
CassNot
left a comment
There was a problem hiding this comment.
We still miss:
- a demo notebook illustrating that this model works as in https://github.com/merlinquantum/reproduced_papers/tree/main/papers/photonic_QGAN with Adam optimizer
- maybe you could add the original image from the paper in the documentation to illustrate how the model works (it is quite clear with it)
There was a problem hiding this comment.
Pull request overview
This PR introduces a new PhotonicGenerator model under merlin.models that composes one or more QuantumLayer heads into a latent-to-sample generator, and adds an OccupancyGrouping policy used by photonic QGAN-style image pipelines. It also wires up the necessary plumbing in QuantumLayer to bind key-aware groupings to layer output keys.
Changes:
- Add
PhotonicGenerator,GeneratorMeasurements,LatentDistribution/NormalLatent, andOutputAdapter/VectorAdapter/ImageAdapterin a newmerlin/models/photonic_generator.py. - Add
OccupancyGroupinginmerlin/utils/grouping.pyplus_bind_grouping_to_output_keys, and integrate output-key binding intoQuantumLayerso probability groupings can refineoutput_keys/output_size. - Export new public APIs from
merlinandmerlin.models, and add Sphinx pages for the new grouping and generator modules.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| merlin/models/photonic_generator.py | New generator model, latent/output-adapter abstractions, and concrete vector/image adapters. |
| merlin/utils/grouping.py | New OccupancyGrouping plus shared key-binding helper. |
| merlin/utils/init.py | Exports OccupancyGrouping. |
| merlin/measurement/strategies.py | Accepts OccupancyGrouping in probability/partial strategies. |
| merlin/algorithms/layer.py | Binds key-aware groupings and uses the bound grouping in forward/output_keys. |
| merlin/models/init.py | Re-exports new generator-related symbols. |
| merlin/init.py | Top-level exports for new public API. |
| tests/models/test_photonic_generator.py | New comprehensive test suite for the generator pipeline. |
| tests/utils/test_grouping.py | Adds OccupancyGrouping test class covering binding, filtering, collapsing, gradients, dtype. |
| tests/measurement/test_measurement_strategy.py | Adds an integration test verifying OccupancyGrouping binds to layer output keys. |
| docs/source/api_reference/api/merlin.utils.grouping.rst | Adds OccupancyGrouping autoclass entry. |
| docs/source/api_reference/api/merlin.models.rst | Adds toctree entry for the new generator page. |
| docs/source/api_reference/api/merlin.models.photonic_generator.rst | New API reference page for the generator and adapters. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@ben9871 there are still some validations to do within the notebook (the loss function are a bit flat so weird) |
CassNot
left a comment
There was a problem hiding this comment.
Approve but will be edited in the release/0.4-photonicQGAN dedicated branch (further validation, comparison, benchmark and scalability study)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c135e631d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
LF-Vigneux
left a comment
There was a problem hiding this comment.
Approved my first quick review, forgot to do it
52b5d54
into
merlinquantum:release/0.4-photonicQGAN
PML-282 Add QuantumLayer-backed PhotonicGenerator
Summary
This PR adds a
PhotonicGeneratormodel that composes one or more existingQuantumLayerobjects into a latent-to-sample generator. It keeps quantum execution insideQuantumLayer, exposes raw per-layer measurements for inspection, and uses output adapters to map measurements to vectors or images.Related Issue
Related Jira: PML-282
Type of change
Proposed changes
merlin.models.PhotonicGenerator.GeneratorMeasurementsto carry raw per-layer outputs and output-key metadata.LatentDistributionNormalLatentOutputAdapterVectorAdapterImageAdaptermerlin.modelsand top-levelmerlin.docs/source/api_reference/api/merlin.models.photonic_generator.rst.API shape:
How to test / How to run
Observed locally:
Screenshots / Logs (optional)
N/A.
Performance considerations (optional)
The generator evaluates its underlying
QuantumLayerheads sequentially and then adapts the collected outputs. Runtime is therefore approximately the sum of the runtimes of the configured quantum layers. No cross-layer batching or caching is introduced in this PR.This is intentional for the first version because different generator heads may use different circuits, input states, output spaces, measurement strategies, and noise models.
Documentation
Checklist
PR title includes Jira issue key (e.g., PML-126)
"Related Jira ticket" section includes the Jira issue key (no URL)
Code formatted (ruff format)
Lint passes (ruff)
Static typing passes (mypy) if applicable
Unit tests added/updated (pytest)
Tests pass locally (pytest)
Tests pass on GPU (pytest)
Test coverage not decreased significantly
Docs build locally if affected (sphinx)
With this command:
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
PR description explains what changed and how to validate it