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

feat: Graph methods for circuit analysis (part 1) #7948

Merged
merged 24 commits into from
Nov 1, 2024
Merged

Conversation

DanielKotov
Copy link
Contributor

Create graph description for Ultra Circuit Builder.

The original idea was to create to directed graph for arithmetic circuit, that bases on Ultra Arithmetization.

The constructor creates graph based on blocks for ultra: arithmetic, elliptic, delta range and lookup. In the time of this process gate counter for every variable depends on gate. Also was created algorithm for finding connected components using depth first search algorithm.

Tests from ultra_curcuit_builder were being used for testing the prototype.

summary: there's a constructor for creating graph for Ultra Circuit Builder with additional algorithm for analyzing Circuit.

@DanielKotov DanielKotov requested a review from Rumata888 August 13, 2024 17:48
@Rumata888 Rumata888 changed the title dk/boomerang value feat: Graph methods for circuit analysis Aug 13, 2024
@Rumata888 Rumata888 changed the title feat: Graph methods for circuit analysis feat: Graph methods for circuit analysis (part 1) Aug 13, 2024
Copy link
Contributor

@Rumata888 Rumata888 left a comment

Choose a reason for hiding this comment

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

Please address the comments and resubmit for review

@@ -473,6 +473,57 @@ void StandardCircuitBuilder_<FF>::fix_witness(const uint32_t witness_index, cons
++this->num_gates;
}

template <typename FF> void StandardCircuitBuilder_<FF>::print_boomerang_variables()
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add comment about how this function works

StandardCircuitBuilder circuit_constructor = StandardCircuitBuilder();
fr a = fr::one();
circuit_constructor.add_public_variable(a);
Graph graph = Graph(circuit_constructor);
Copy link
Contributor

Choose a reason for hiding this comment

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

You are not checking graph structure in any of these tests, so they aren't doing anything. Please add checks of graph structure

}

template <typename FF>
void Graph_<FF>::connect_all_variables_in_vector(const std::vector<uint32_t>& variables_vector,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you still using zero index if you need to check that it's not a constant variable?

Copy link
Collaborator

@AztecBot AztecBot left a comment

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'C++ Benchmark'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.05.

Benchmark suite Current: 2744ba4 Previous: 7f6dba2 Ratio
nativeconstruct_proof_ultrahonk_power_of_2/20 5168.177880000002 ms/iter 4750.393791999997 ms/iter 1.09
commit(t) 4428680062 ns/iter 3767372954 ns/iter 1.18

This comment was automatically generated by workflow using github-action-benchmark.

CC: @ludamad @codygunton

Copy link
Contributor

github-actions bot commented Aug 13, 2024

Changes to circuit sizes

Generated at commit: 163db00603fa968ad17c926e17dfbbeba68c0f9d, compared to commit: 86b249022167762fb9558aedf579f5648097592a

🧾 Summary (100% most significant diffs)

Program ACIR opcodes (+/-) % Circuit size (+/-) %
rollup_block_merge 0 ➖ 0.00% -12 ✅ -0.00%
rollup_root 0 ➖ 0.00% -12 ✅ -0.00%
rollup_block_root 0 ➖ 0.00% -24 ✅ -0.00%
parity_root 0 ➖ 0.00% -36 ✅ -0.00%
rollup_merge 0 ➖ 0.00% -24 ✅ -0.00%
rollup_base_private 0 ➖ 0.00% -1,476 ✅ -0.05%
rollup_base_public 0 ➖ 0.00% -1,476 ✅ -0.06%
parity_base 0 ➖ 0.00% -36 ✅ -0.12%

Full diff report 👇
Program ACIR opcodes (+/-) % Circuit size (+/-) %
rollup_block_merge 5,057 (0) 0.00% 1,911,809 (-12) -0.00%
rollup_root 5,041 (0) 0.00% 1,911,795 (-12) -0.00%
rollup_block_root 4,021 (0) 0.00% 2,857,288 (-24) -0.00%
parity_root 5,031 (0) 0.00% 3,801,549 (-36) -0.00%
rollup_merge 3,415 (0) 0.00% 1,909,442 (-24) -0.00%
rollup_base_private 345,125 (0) 0.00% 3,272,730 (-1,476) -0.05%
rollup_base_public 345,984 (0) 0.00% 2,314,712 (-1,476) -0.06%
parity_base 4,298 (0) 0.00% 30,695 (-36) -0.12%

@DanielKotov DanielKotov requested a review from Rumata888 August 23, 2024 15:21
@DanielKotov DanielKotov requested a review from Sarkoxed October 9, 2024 12:08
@DanielKotov DanielKotov requested a review from Sarkoxed October 21, 2024 14:37
}

/**
* @brief this method removes false cases in lookup table for a given gate
Copy link
Contributor

Choose a reason for hiding this comment

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

yet another false cases occurrence

Copy link
Contributor

@Sarkoxed Sarkoxed left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@Rumata888 Rumata888 left a comment

Choose a reason for hiding this comment

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

We'll need to document the tests and functionality more, but let's merge it in for now

@Rumata888 Rumata888 enabled auto-merge (squash) November 1, 2024 21:42
@Rumata888 Rumata888 merged commit eeea55a into master Nov 1, 2024
46 checks passed
@Rumata888 Rumata888 deleted the dk/boomerang_value branch November 1, 2024 22:12
TomAFrench added a commit that referenced this pull request Nov 4, 2024
* master: (81 commits)
  feat: Encode static error strings in the ABI (#9552)
  chore: redo typo PR by donatik27 (#9693)
  chore: update install instructions for foundry to display version of rust to install (#9653)
  chore: disable bench upload until #9692
  fix: earthly-ci in bench-e2e (#9689)
  chore: redo typo PR by cypherpepe (#9687)
  chore: redo typo PR by youyyytrok (#9686)
  chore: redo typo PR by mdqst (#9685)
  chore: redo typo PR by mdqst (#9684)
  feat: adding tags to encrypted logs (#9566)
  fix: enable gerousia e2e test (#9677)
  git subrepo push --branch=master noir-projects/aztec-nr
  git_subrepo.sh: Fix parent in .gitrepo file. [skip ci]
  chore: replace relative paths to noir-protocol-circuits
  git subrepo push --branch=master barretenberg
  chore: redo typo PR by dsarfed (#9667)
  fix: bench e2e jobs (#9662)
  fix: Fix random for Mac users  (#9670)
  feat: Graph methods for circuit analysis (part 1) (#7948)
  feat: Faster random sampling (#9655)
  ...
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.

4 participants