Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deep copy from Program.compile() #688

Merged
merged 9 commits into from
Mar 11, 2022
Merged

Deep copy from Program.compile() #688

merged 9 commits into from
Mar 11, 2022

Conversation

thisac
Copy link
Contributor

@thisac thisac commented Mar 9, 2022

Context:
The compiler create a linked copy of the program by calling Program._linked_copy(), which retains the same regrefs. Since the copy is shallow, it also keeps the same TDM parameters in a TDM program. These parameters are updated by the compiler and thus changed in both the compiled program and the initial program, while the circuit is only changed in the compiled program.

Description of the Change:

  • Program._linked_copy() is updated to create a deep-copy of all attributes except the ones containing register references, including e.g., the TDM parameters.
  • The initial program's regrefs are linked to the compiled programs regrefs instead of implicitly referenced by using a shallow copy.

Benefits:
The compiler actually returns a copy of the program without changing anything in the original program.

Possible Drawbacks:
None

Related GitHub Issues:
None

@thisac thisac requested a review from josh146 March 9, 2022 20:48
@thisac thisac self-assigned this Mar 9, 2022
@codecov
Copy link

codecov bot commented Mar 10, 2022

Codecov Report

Merging #688 (d6665b6) into master (efdf123) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #688   +/-   ##
=======================================
  Coverage   98.69%   98.69%           
=======================================
  Files          75       75           
  Lines        9209     9212    +3     
=======================================
+ Hits         9089     9092    +3     
  Misses        120      120           
Impacted Files Coverage Δ
strawberryfields/program.py 100.00% <100.00%> (ø)

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 efdf123...d6665b6. Read the comment docs.

Comment on lines +555 to +556
if name not in ("circuit", "reg_refs", "init_reg_refs"):
setattr(p, name, copy.deepcopy(val))
Copy link
Contributor Author

@thisac thisac Mar 10, 2022

Choose a reason for hiding this comment

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

Optimally, we should deep-copy the circuit as well, but since the may contain regrefs, this isn't as straight-forward to do. Just making a deepcopy here would cause two errors:

  1. Due to MeasuredParameter not supporting deepcopy (because of the different signature of the __new__() method). A __deepcopy__() method would potentially need to be added.
  2. Any copied (not referenced) regrefs would raise the unnerving "RegRef state has become inconsistent." issue we had earlier.

We could e.g., override the __deepcopy__ method for the Command class, but that would require it to only deep-copy everything except symbolic parameters. Alternatively, add a copy_everything_except_regref method to Command.

Copy link
Contributor

@sduquemesa sduquemesa Mar 10, 2022

Choose a reason for hiding this comment

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

Because this is a potentially actionable item, perhaps is worth to have it as a (TODO) comment in the code. It might get lost here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've created an issue for this instead (#691). I think it's better to keep track of it there rather than as a TODO which can easily be lost. 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've created an issue for this instead (#691). I think it's better to keep track of it there rather than as a TODO which can easily be lost. 🙂

@thisac thisac requested a review from sduquemesa March 10, 2022 14:45
Copy link
Contributor

@sduquemesa sduquemesa left a comment

Choose a reason for hiding this comment

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

Great @thisac!
One question only: why programs should share the same references for the registers? Why copy everything else except reg_refs?

tests/frontend/test_program.py Outdated Show resolved Hide resolved
@thisac thisac merged commit 72d4fdc into master Mar 11, 2022
@thisac thisac deleted the sc-15758-deep-copy-compile branch March 11, 2022 19:47
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