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

[ECC] Cost enhancements #1405

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mpharrigan
Copy link
Collaborator

  • sundry improvements to the cost expressions, including better comments on where they're from
  • example using the real 256 bit modulus, which reveals a problem with ModAdd(?)
  • fixes for places where a symbolic value gets passed but the decomposition doesn't account for it

@NoureldinYosri can you take a look

# TODO https://github.com/quantumlib/Qualtran/issues/1261
if cost_key == QubitCount():
return self.n + 1
return NotImplemented


@frozen
Copy link
Contributor

Choose a reason for hiding this comment

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

there is no need for this shim. you can use the LinearDepthGreaterThan bloq

return NotImplemented


@frozen
Copy link
Contributor

Choose a reason for hiding this comment

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

why a shim? why not use the QROM bloq itself (with Shaped as data)?

return {ECAddR(n=self.n, R=self.point): self.n, MeasureQFT(n=self.n): 1}
def build_call_graph(self, ssa: 'SympySymbolAllocator') -> Set['BloqCountT']:
return {
(self.ec_add(R=self.point), self.n / (2**self.window_size)),
Copy link
Contributor

Choose a reason for hiding this comment

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

float division?

@@ -132,8 +142,11 @@ def signature(self) -> 'Signature':
)

def build_call_graph(self, ssa: 'SympySymbolAllocator') -> 'BloqCountDictT':
# Roetteler montgomery
return {Toffoli(): ceil(16 * self.n**2 * log2(self.n) - 26.3 * self.n**2)}
# Roetteler 2017 montgomery multiplier
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want to keep this comment?

@@ -115,7 +116,8 @@ def build_composite_bloq(self, bb: 'BloqBuilder', x: Soquet, y: Soquet) -> Dict[
x = bb.join(x_split[1:], dtype=QMontgomeryUInt(bitsize=self.bitsize))

# Add constant -p to the y register.
y = bb.add(AddK(bitsize=self.bitsize + 1, k=-1 * self.mod, signed=True, cvs=()), x=y)
# FIXME: signed addition not large enough to fit `self.bitsize`-ed number.
Copy link
Contributor

Choose a reason for hiding this comment

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

this addition is unsiged ... it doesn't need fixing ... the flag sigend=True was wrong and you just fixed it... AddK will do the right thing for -p

val = self.k
if val < 0:
# Since this is unsigned addition adding -v is equivalent to
# adding 2^bitsize - v
val %= 2**self.bitsize
binary_rep = QUInt(self.bitsize).to_bits(val)

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