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

fix(stdlib): correct tecurve::contains formula #1821

Merged
merged 3 commits into from
Aug 9, 2023

Conversation

shuklaayush
Copy link
Contributor

Description

The tecurve::contains formula for extended coordinates should be

$$ z^2\left(ax^2 + y^2\right) = z^{\color{red}{4}} + dx^2y^2 $$

Some other changes:

  • Simplify equality/zero checks
  • Clean up boolean checks

cc @ax0

Problem*

Resolves

Summary*

Documentation

  • This PR requires documentation updates when merged.

    • I will submit a noir-lang/docs PR.
    • I will request for and support Dev Rel's help in documenting this PR.

Additional Context

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@shuklaayush
Copy link
Contributor Author

Not sure why the test is failing. The non ssa_refactor one seems to be passing

noir_stdlib/src/ec/tecurve.nr Outdated Show resolved Hide resolved
@jfecher
Copy link
Contributor

jfecher commented Jun 26, 2023

Not sure why the test is failing. The non ssa_refactor one seems to be passing

Sorry about that, I'll look into it

@shuklaayush shuklaayush force-pushed the fix/tecurve-contains branch from 97b438f to aa99176 Compare June 27, 2023 05:53
@TomAFrench
Copy link
Member

Looks like the test failure was due to #2099 which has now been resolved.

@shuklaayush Can you merge master into this branch so we can take a look at this again?

@shuklaayush
Copy link
Contributor Author

@TomAFrench Done. Passing now :)

@kevaundray kevaundray requested a review from ax0 August 8, 2023 21:04
@kevaundray
Copy link
Contributor

@ax0 Were you happ with the changes made here?

Copy link
Contributor

@ax0 ax0 left a comment

Choose a reason for hiding this comment

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

LGTM

@kevaundray kevaundray added this pull request to the merge queue Aug 9, 2023
Merged via the queue into noir-lang:master with commit 6a10ecf Aug 9, 2023
@shuklaayush shuklaayush deleted the fix/tecurve-contains branch August 9, 2023 08:13
TomAFrench added a commit that referenced this pull request Aug 10, 2023
* master: (80 commits)
  fix: properly capture lvalues in closure environments (#2120) (#2257)
  fix: Optimize contracts built by `nargo info` (#2259)
  chore: impl Display for DebugType (#2258)
  chore: update `noir_wasm` build process to match `acvm_js` (#2067)
  feat: Implement traits - parser support #2094 (#2230)
  chore: Refactor DefCollector duplicate errors (#2231)
  chore: Address clippy warnings (#2246)
  feat: Support `contract` package type in `nargo info` command (#2249)
  feat: Add slice append (#2241)
  chore: Bump `async-lsp` to v0.0.5 (#2186)
  chore: Move the remaining `nargo_cli` lib funcs into `nargo` crate (#2225)
  chore: Add test for eddsa (#2237)
  chore: Split `Nargo.toml` operations into separate package (#2224)
  fix(stdlib): correct `tecurve::contains` formula (#1821)
  feat: Remove `comptime` and warn upon usage (#2178)
  fix: Remove last vestige of array of structs to struct of arrays conversion (#2217)
  fix: Add foreign impl error (#2216)
  feat(nargo)!: Replace `--contracts` flag with `contract` package type (#2204)
  feat: Optimize `x < 0` for unsigned `x` to false (#2206)
  fix: Initialize numeric generics' type to a polymorphic integer when used in an expression (#2179)
  ...
TomAFrench added a commit that referenced this pull request Aug 11, 2023
* master:
  feat: Add slice append (#2241)
  chore: Bump `async-lsp` to v0.0.5 (#2186)
  chore: Move the remaining `nargo_cli` lib funcs into `nargo` crate (#2225)
  chore: Add test for eddsa (#2237)
  chore: Split `Nargo.toml` operations into separate package (#2224)
  fix(stdlib): correct `tecurve::contains` formula (#1821)
  feat: Remove `comptime` and warn upon usage (#2178)
  fix: Remove last vestige of array of structs to struct of arrays conversion (#2217)
  fix: Add foreign impl error (#2216)
  feat(nargo)!: Replace `--contracts` flag with `contract` package type (#2204)
  feat: Optimize `x < 0` for unsigned `x` to false (#2206)
  fix: Initialize numeric generics' type to a polymorphic integer when used in an expression (#2179)
  chore(nargo)!: remove `flat_witness` feature flag (#2208)
  chore: Revert "feat: Only create new witnesses for distinctiveness when duplicates exist" (#2209)
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.

5 participants