antalszava commented 4 years ago

Context: The Catstate gate is the last Operation that accepts complex arguments upon being created. This change is related to PRs #378 #428 #439, where other gates were changed to take real arguments.

Description of the Change:

Changes the Catstate gate and the cat_state function in the utils package to take real arguments instead of complex ones.
Creates a _check_for_complex_args and _create_ket convenience methods to remove complexity from the Catstate._apply method. _check_for_complex_args has been used for other gates too.

Benefits: There are no gates remaining that take complex parameters.

Possible Drawbacks: The learning curve for users for picking up the new signature of Catstate.

Related GitHub Issues: N/A

Linting locally seems to indicate that codefactor has a complaint due to other parts of the ops.py file:

************* Module strawberryfields.ops
strawberryfields/ops.py:43:2: W0511: TODO this tolerance is used for various purposes and is not well-defined (fixme)
strawberryfields/ops.py:155:2: W0511: todo: Using the return value None to denote the identity is a (fixme)
strawberryfields/ops.py:280:2: W0511: todo: self.select could support :class:`~strawberryfields.parameters.Parameter` instances. (fixme)
strawberryfields/ops.py:394:2: W0511: TODO decide how all Channels should treat the first parameter p[0] (fixme)
strawberryfields/ops.py:2307:4: R1260: '_decompose' is too complex. The McCabe rating is 12 (too-complex)
strawberryfields/ops.py:2734:4: R1260: '_decompose' is too complex. The McCabe rating is 14 (too-complex)

------------------------------------------------------------------
Your code has been rated at 9.91/10 (previous run: 9.94/10, -0.03)

codecov[bot] commented 4 years ago

Codecov Report

Merging #441 (5cec2ad) into master (0e06dc6) will increase coverage by 0.02%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #441      +/-   ##
==========================================
+ Coverage   98.55%   98.57%   +0.02%     
==========================================
  Files          77       77              
  Lines        8989     8999      +10     
==========================================
+ Hits         8859     8871      +12     
+ Misses        130      128       -2

Impacted Files	Coverage Δ
strawberryfields/parameters.py	`100.00% <ø> (ø)`
strawberryfields/api/devicespec.py	`95.31% <100.00%> (ø)`
strawberryfields/backends/base.py	`89.16% <100.00%> (+0.09%)`	:arrow_up:
...trawberryfields/backends/bosonicbackend/backend.py	`100.00% <100.00%> (ø)`
strawberryfields/backends/fockbackend/backend.py	`100.00% <100.00%> (ø)`
...rawberryfields/backends/gaussianbackend/backend.py	`98.46% <100.00%> (+0.01%)`	:arrow_up:
strawberryfields/backends/states.py	`99.84% <100.00%> (ø)`
strawberryfields/backends/tfbackend/__init__.py	`73.33% <100.00%> (ø)`
strawberryfields/backends/tfbackend/backend.py	`100.00% <100.00%> (ø)`
strawberryfields/backends/tfbackend/circuit.py	`96.40% <100.00%> (+0.38%)`	:arrow_up:
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 0e06dc6...5cec2ad. Read the comment docs.

nquesada commented 3 years ago

Should this PR still be open @antalszava ?

antalszava commented 3 years ago

As far as I know, it's still relevant. The last step here would be to resolve the conflicts and get it through the review round.

sduquemesa commented 3 years ago

Hi @antalszava, @thisac, @josh146, I believe this is ready for a final review round before merging! 🔍

CodeFactor is showing a lot of issues that did not appear before I merged master and solved the conflicts. Those warnings are either coming from master (unlikely) or CodeFactor became more strict since the last @antalszava's commit (around March). Should I apply the suggested changes before merging?

antalszava commented 3 years ago

Hi @sduquemesa, thank you! The parts of the code that Codefactor warns about were changed outside of this PR. We could override the check because resolving them is not within the scope of this PR.

Maybe it's best to apply the suggested changes in a separate PR.

sduquemesa commented 3 years ago

@antalszava, thanks for your review! Any other suggestion before merge? 🚀

antalszava commented 3 years ago

@sduquemesa this looks good to me, thank you so much for the updates! :1st_place_medal: :blush:

sduquemesa commented 3 years ago

Thanks @antalszava, glad I can help! Will await for @josh146 or @thisac review and approval.

XanaduAI / strawberryfields

Making Catstate take real arguments #441

Codecov Report