Skip to content

Conversation

@hzhangxyz
Copy link
Member

No description provided.

@hzhangxyz hzhangxyz requested a review from Copilot July 30, 2025 16:45

This comment was marked as outdated.

@hzhangxyz hzhangxyz force-pushed the dev/add-parity-and-mask branch from 91b028a to ae77625 Compare July 30, 2025 16:48
@hzhangxyz hzhangxyz requested a review from Copilot July 30, 2025 16:49
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the ParityTensor class to add a new parity field and improve encapsulation by making fields private with public property accessors. The changes enable lazy computation of parity and mask values while preserving them during arithmetic operations for better performance.

Key changes:

  • Made all fields private (_edges, _tensor, _parity, _mask) with public property accessors
  • Added lazy computation for parity and mask properties with caching
  • Updated all arithmetic operators to preserve _parity and _mask references from the left operand

Comment on lines 111 to +115
return ParityTensor(
edges=self.edges,
tensor=self.tensor + other.tensor,
_edges=self._edges,
_tensor=self._tensor + other._tensor,
_parity=self._parity,
_mask=self._mask,
Copy link

Copilot AI Jul 30, 2025

Choose a reason for hiding this comment

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

The mask from self is being copied to the result of addition with another ParityTensor, but the result tensor may have different values that could invalidate this mask. The mask should be recomputed or set to None to ensure correctness.

Copilot uses AI. Check for mistakes.
_edges=self._edges,
_tensor=self._tensor - other._tensor,
_parity=self._parity,
_mask=self._mask,
Copy link

Copilot AI Jul 30, 2025

Choose a reason for hiding this comment

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

The mask from self is being copied to the result of subtraction with another ParityTensor, but the result tensor may have different values that could invalidate this mask. The mask should be recomputed or set to None to ensure correctness.

Suggested change
_mask=self._mask,
_mask=None,

Copilot uses AI. Check for mistakes.
_edges=self._edges,
_tensor=self._tensor * other._tensor,
_parity=self._parity,
_mask=self._mask,
Copy link

Copilot AI Jul 30, 2025

Choose a reason for hiding this comment

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

The mask from self is being copied to the result of multiplication with another ParityTensor, but element-wise multiplication can change which elements should be zero according to the parity rules. The mask should be recomputed or set to None to ensure correctness.

Suggested change
_mask=self._mask,
_mask=None,

Copilot uses AI. Check for mistakes.
_edges=self._edges,
_tensor=self._tensor / other._tensor,
_parity=self._parity,
_mask=self._mask,
Copy link

Copilot AI Jul 30, 2025

Choose a reason for hiding this comment

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

The mask from self is being copied to the result of division with another ParityTensor, but division can change which elements should be zero according to the parity rules. The mask should be recomputed or set to None to ensure correctness.

Suggested change
_mask=self._mask,
_mask=None,

Copilot uses AI. Check for mistakes.
@hzhangxyz hzhangxyz merged commit 48f6df6 into main Jul 30, 2025
6 checks passed
@hzhangxyz hzhangxyz mentioned this pull request Jul 30, 2025
13 tasks
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