quantumlib / Cirq

A Python framework for creating, editing, and invoking Noisy Intermediate Scale Quantum (NISQ) circuits.
Apache License 2.0
4.28k stars 1.02k forks source link

Update ClassicalSimulator to use simulation infrastructure #6432

Closed shef4 closed 8 months ago

shef4 commented 9 months ago

Related issue - #6343

NoureldinYosri commented 9 months ago

@shef4 to fix the coverage failure you need to add more tests. tests that will cover the new lines you added, specifically these lines

************* cirq-core/cirq/sim/classical_simulator.py (4 uncovered)
Line   69 is new or changed but not covered:          return ClassicalStateTrialResult(
Line   70 is new or changed but not covered:              params, measurements, final_simulator_state=final_simulator_state
Line   71 is new or changed but not covered:          )
Line   76 is new or changed but not covered:          return ClassicalStateStepResult(sim_state)

Found 4 uncovered touched lines.
shef4 commented 9 months ago

@NoureldinYosri Thanks for the insight, looking for example test to follow 👍

review-notebook-app[bot] commented 9 months ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

codecov[bot] commented 9 months ago

Codecov Report

Attention: Patch coverage is 99.20000% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 97.76%. Comparing base (7780c01) to head (064ae51).

Files Patch % Lines
cirq-core/cirq/sim/classical_simulator.py 98.41% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #6432 +/- ## ======================================= Coverage 97.75% 97.76% ======================================= Files 1105 1105 Lines 95047 95130 +83 ======================================= + Hits 92915 93001 +86 + Misses 2132 2129 -3 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

shef4 commented 9 months ago

Hi @NoureldinYosri, in the Qualtran meeting I think it was mentioned that Classical Simulators are signed int types.

Would it be better to use the original _run() function and set state_type = Type[int] or state_type = type(int) as a default value in the constructor? - All tests pass except for the ones I added, might cause coverage issues.

NoureldinYosri commented 9 months ago

these are two different things the state_type is the name of the class that will store the simulation state, this type should be a subclass of SimulationState class.

the conversation in the qualtran sync was about quantum number/registers. quantum numbers aren't represented explicitly in Cirq. Cirq deals with the qubits directly while Qulatran groups qubits to form registers that can then be used to represent numbers which can be signed/unsigned, integer/float, ..etc. Cirq and Qualtran are two different libraries and have different scopes.

shef4 commented 8 months ago

Hi @NoureldinYosri, I'm not sure what is causing this CI fail, any insight would be really appreciated.

FAILED dev_tools/docker_test.py::test_docker_pre - assert 1 == 0
 +  where 1 = CompletedProcess(args=['docker run cirq_image_pre python -c "import cirq; assert cirq.__version__ is not None"'], returncode=1).returncode
= 1 failed, 20135 passed, 146 skipped, 86 xfailed, 726 warnings in 365.34s (0:06:05) =
NoureldinYosri commented 8 months ago

@shef4 I temporarily supressed that test because it is failing at head.

shef4 commented 8 months ago

@NoureldinYosri Thanks, glad to know it's working. Learnt some interesting concepts from this issue!