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.

Sign up for GitHub

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

Jump to bottom

Remove public fields from gates #4851

Closed
13 tasks done
daxfohl opened this issue Jan 17, 2022 · 1 comment · Fixed by #5075
Closed
13 tasks done

Remove public fields from gates #4851

daxfohl opened this issue Jan 17, 2022 · 1 comment · Fixed by #5075
Labels
area/gates kind/design-issue A conversation around design triage/accepted A consensus emerged that this bug report, feature request, or other action should be worked on

Comments

@daxfohl
Copy link
Collaborator

daxfohl commented Jan 17, 2022 •
edited
Loading

We still have a number of gates and operations that have public fields. These should be private, with public getters if necessary. (And those getters should not return mutable references unless it's something performance critical.) Otherwise this breaks the contracts of FrozenCircuit etc.

Ideally upon fixing this we should create a unit test that checks this to some extent for all gates in Cirq.

This should probably be done before 1.0.

(cirq-py3) dax@DESKTOP-Q5MLJ3J:~/cirq$ grep -r "self\.[^_][^\ ]*\s=\s" --exclude "*test.py" cirq-core/cirq/ops/*
Binary file cirq-core/cirq/ops/__pycache__/arithmetic_operation.cpython-38.pyc matches
Binary file cirq-core/cirq/ops/__pycache__/arithmetic_operation.cpython-39.pyc matches
cirq-core/cirq/ops/controlled_gate.py:        self.control_qid_shape = tuple(control_qid_shape)
cirq-core/cirq/ops/controlled_gate.py:        self.control_values = cast(
cirq-core/cirq/ops/controlled_gate.py:            self.sub_gate = sub_gate.sub_gate  # type: ignore
cirq-core/cirq/ops/controlled_gate.py:            self.sub_gate = sub_gate
cirq-core/cirq/ops/controlled_operation.py:        self.control_values = cast(
cirq-core/cirq/ops/controlled_operation.py:            self.controls = tuple(controls)
cirq-core/cirq/ops/controlled_operation.py:            self.sub_operation = sub_operation
cirq-core/cirq/ops/controlled_operation.py:            self.controls = tuple(controls) + sub_operation.controls
cirq-core/cirq/ops/controlled_operation.py:            self.sub_operation = sub_operation.sub_operation
cirq-core/cirq/ops/dense_pauli_string.py:        self.pauli_mask = _as_pauli_mask(pauli_mask)
cirq-core/cirq/ops/dense_pauli_string.py:        self.coefficient = (
cirq-core/cirq/ops/dense_pauli_string.py:            self.pauli_mask = np.copy(self.pauli_mask)
cirq-core/cirq/ops/dense_pauli_string.py:            self.pauli_mask[key] = _pauli_index(value)
cirq-core/cirq/ops/dense_pauli_string.py:                self.pauli_mask[key] = value.pauli_mask
cirq-core/cirq/ops/dense_pauli_string.py:                self.pauli_mask[key] = _as_pauli_mask(value)
cirq-core/cirq/ops/dense_pauli_string.py:            self.coefficient = new_coef if isinstance(new_coef, sympy.Basic) else complex(new_coef)
cirq-core/cirq/ops/fourier_transform.py:        self.exponent = exponent
cirq-core/cirq/ops/fsim_gate.py:        self.theta = _canonicalize(theta)
cirq-core/cirq/ops/fsim_gate.py:        self.phi = _canonicalize(phi)
cirq-core/cirq/ops/fsim_gate.py:        self.theta = _canonicalize(theta)
cirq-core/cirq/ops/fsim_gate.py:        self.zeta = _canonicalize(zeta)
cirq-core/cirq/ops/fsim_gate.py:        self.chi = _canonicalize(chi)
cirq-core/cirq/ops/fsim_gate.py:        self.gamma = _canonicalize(gamma)
cirq-core/cirq/ops/fsim_gate.py:        self.phi = _canonicalize(phi)
cirq-core/cirq/ops/measurement_gate.py:        self.key = key  # type: ignore
cirq-core/cirq/ops/measurement_gate.py:        self.invert_mask = invert_mask or ()
cirq-core/cirq/ops/measurement_gate.py:            self.mkey = key
cirq-core/cirq/ops/measurement_gate.py:            self.mkey = value.MeasurementKey(name=key)
cirq-core/cirq/ops/pauli_interaction_gate.py:        self.pauli0 = pauli0
cirq-core/cirq/ops/pauli_interaction_gate.py:        self.invert0 = invert0
cirq-core/cirq/ops/pauli_interaction_gate.py:        self.pauli1 = pauli1
cirq-core/cirq/ops/pauli_interaction_gate.py:        self.invert1 = invert1
cirq-core/cirq/ops/pauli_measurement_gate.py:        self.key = key  # type: ignore
cirq-core/cirq/ops/pauli_measurement_gate.py:        self.mkey = key
cirq-core/cirq/ops/pauli_string_raw_types.py:        self.pauli_string = pauli_string
cirq-core/cirq/ops/permutation_gate.py:        self.permutation = tuple(permutation)
cirq-core/cirq/ops/random_gate_channel.py:        self.sub_gate = sub_gate
cirq-core/cirq/ops/random_gate_channel.py:        self.probability = probability
cirq-core/cirq/ops/random_gate_channel.py:            self.sub_gate = self.sub_gate.sub_gate
cirq-core/cirq/ops/raw_types.py:        self.sub_operation = sub_operation
cirq-core/cirq/ops/wait_gate.py:        self.duration = value.Duration(duration)
@daxfohl daxfohl added the kind/design-issue A conversation around design label Jan 17, 2022
@viathor viathor added area/gates triage/discuss Needs decision / discussion, bring these up during Cirq Cynque labels Jan 18, 2022
@tanujkhattar tanujkhattar added triage/accepted A consensus emerged that this bug report, feature request, or other action should be worked on and removed triage/discuss Needs decision / discussion, bring these up during Cirq Cynque labels Jan 26, 2022
@tanujkhattar
Copy link
Collaborator

From cirq sync:

We should do it. Let's do a deprecation by creating a setter and mark it deprecated.

@daxfohl daxfohl mentioned this issue Feb 22, 2022
Merged
CirqBot pushed a commit that referenced this issue Feb 22, 2022
@daxfohl
Replace fields with properties in ActOnArgs (#5011)
ca4c8e7 
xref #4851 @95-martin-orion
95-martin-orion pushed a commit to 95-martin-orion/Cirq that referenced this issue Mar 2, 2022
@daxfohl @95-martin-orion
Replace fields with properties in ActOnArgs (quantumlib#5011)
f8634f0 
xref quantumlib#4851 @95-martin-orion
This was referenced Mar 8, 2022
Merged
Merged
Merged
Merged
Merged
CirqBot pushed a commit that referenced this issue Mar 13, 2022
@daxfohl
Remove public fields for measurement and controlled ops (#5061)
120f296 
Change public fields to private, and encapsulate with getters and deprecated setters.

xref #4851
CirqBot pushed a commit that referenced this issue Mar 13, 2022
@daxfohl
Remove public fields from subop gates and ops (#5074)
097544e 
Change public fields to private, and encapsulate with getters and deprecated setters.

xref #4851
CirqBot pushed a commit that referenced this issue Mar 13, 2022
@daxfohl
Wrap private fields for DensePauliString (#5064)
27f92ae 
#4851 for `DensePauliString`. This was *slightly* less straightforward than others.

* Required moving `ALLOW_DEPRECATION_IN_TEST` or else a circular dependency was created.
* One place externally where setting the field still was necessary. (A factory method in `CliffordGate`).
* One member is of type `ndarray`, which is mutable, so we set `flags.writeable = False`
CirqBot pushed a commit that referenced this issue Mar 13, 2022
@daxfohl
Remove public fields for pauli ops (#5062)
d8652be 
#4851 for all the pauli stuff.

I skipped BaseDensePauliString because including `cirq._compat` there makes a weird circular dependency.
CirqBot pushed a commit that referenced this issue Mar 13, 2022
@daxfohl
Remove public fields from fsim (#5075)
bfc04b8 
Change public fields to private, and encapsulate with getters and deprecated setters.

Closes #4851
rht pushed a commit to rht/Cirq that referenced this issue May 1, 2023
@daxfohl @rht
Replace fields with properties in ActOnArgs (quantumlib#5011)
05cb1b9 
xref quantumlib#4851 @95-martin-orion
rht pushed a commit to rht/Cirq that referenced this issue May 1, 2023
@daxfohl @rht
Remove public fields for measurement and controlled ops (quantumlib#5061
ca471dd 
)

Change public fields to private, and encapsulate with getters and deprecated setters.

xref quantumlib#4851
rht pushed a commit to rht/Cirq that referenced this issue May 1, 2023
@daxfohl @rht
Remove public fields from subop gates and ops (quantumlib#5074)
9e4123f 
Change public fields to private, and encapsulate with getters and deprecated setters.

xref quantumlib#4851
rht pushed a commit to rht/Cirq that referenced this issue May 1, 2023
@daxfohl @rht
Wrap private fields for DensePauliString (quantumlib#5064)
a109ee8 
quantumlib#4851 for `DensePauliString`. This was *slightly* less straightforward than others.

* Required moving `ALLOW_DEPRECATION_IN_TEST` or else a circular dependency was created.
* One place externally where setting the field still was necessary. (A factory method in `CliffordGate`).
* One member is of type `ndarray`, which is mutable, so we set `flags.writeable = False`
rht pushed a commit to rht/Cirq that referenced this issue May 1, 2023
@daxfohl @rht
Remove public fields for pauli ops (quantumlib#5062)
58e5d88 
quantumlib#4851 for all the pauli stuff.

I skipped BaseDensePauliString because including `cirq._compat` there makes a weird circular dependency.
rht pushed a commit to rht/Cirq that referenced this issue May 1, 2023
@daxfohl @rht
Remove public fields from fsim (quantumlib#5075)
05c2db2 
Change public fields to private, and encapsulate with getters and deprecated setters.

Closes quantumlib#4851
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this issue Oct 31, 2024
@daxfohl
Replace fields with properties in ActOnArgs (quantumlib#5011)
cfe4339 
xref quantumlib#4851 @95-martin-orion
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this issue Oct 31, 2024
@daxfohl
Remove public fields for measurement and controlled ops (quantumlib#5061
4c22d93 
)

Change public fields to private, and encapsulate with getters and deprecated setters.

xref quantumlib#4851
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this issue Oct 31, 2024
@daxfohl
Remove public fields from subop gates and ops (quantumlib#5074)
6143210 
Change public fields to private, and encapsulate with getters and deprecated setters.

xref quantumlib#4851
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this issue Oct 31, 2024
@daxfohl
Wrap private fields for DensePauliString (quantumlib#5064)
e4ac8e6 
quantumlib#4851 for `DensePauliString`. This was *slightly* less straightforward than others.

* Required moving `ALLOW_DEPRECATION_IN_TEST` or else a circular dependency was created.
* One place externally where setting the field still was necessary. (A factory method in `CliffordGate`).
* One member is of type `ndarray`, which is mutable, so we set `flags.writeable = False`
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this issue Oct 31, 2024
@daxfohl
Remove public fields for pauli ops (quantumlib#5062)
e1c3e49 
quantumlib#4851 for all the pauli stuff.

I skipped BaseDensePauliString because including `cirq._compat` there makes a weird circular dependency.
harry-phasecraft pushed a commit to PhaseCraft/Cirq that referenced this issue Oct 31, 2024
@daxfohl
Remove public fields from fsim (quantumlib#5075)
2dadcb3 
Change public fields to private, and encapsulate with getters and deprecated setters.

Closes quantumlib#4851
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area/gates kind/design-issue A conversation around design triage/accepted A consensus emerged that this bug report, feature request, or other action should be worked on
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Remove public fields from fsim daxfohl/Cirq
3 participants
@daxfohl @tanujkhattar @viathor