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: Compute program diffs based off of package name and reset reference report #3

Merged
merged 20 commits into from
Apr 25, 2024

Conversation

vezenovm
Copy link
Contributor

The current gate diff checks has a bug where it indexes the first program rather than the first circuit in every program. I switched the computeProgramDiffs method to find common report names based off of the package name now and a ProgramReport.

github-merge-queue bot pushed a commit to noir-lang/noir that referenced this pull request Apr 10, 2024
# Description

## Problem\*

Resolves [<!-- Link to GitHub Issue
-->](#4764 (comment))

## Summary\*

This is the noir-gates-diff PR this commit update references:
noir-lang/noir-gates-diff#3.


## Additional Context



## Documentation\*

Check one:
- [ ] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[For Experimental Features]** Documentation to be submitted in a
separate PR.

# PR Checklist\*

- [ ] I have tested the changes locally.
- [ ] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.
@TomAFrench
Copy link
Member

Ah sorry, I didn't see this PR existed until now.

@vezenovm
Copy link
Contributor Author

We reference it here though still https://github.com/noir-lang/noir/blob/19583ba2307afcbe5678b470468175057d1f99f0/.github/workflows/gates_report.yml#L77C40-L77C80 and I was expecting this PR to be the fix. But I guess there is still a bug

@vezenovm vezenovm changed the title fix: Compute program diffs based off of package name fix: Compute program diffs based off of package name and reset reference report Apr 23, 2024
github-merge-queue bot pushed a commit to noir-lang/noir that referenced this pull request Apr 23, 2024
# Description

## Problem\*

Resolves <!-- Link to GitHub Issue -->

Opened this PR to test
noir-lang/noir-gates-diff#3 after seeing this
#4848 (comment)

## Summary\*

The noir-gates-diff report became malformed on master as "main" was used
as the program name for each program in `execution_success`. This PR
references the last commit in this PR
noir-lang/noir-gates-diff#3. That PR changes to
using the correct name for each program in `execution_success` but also
resets the report on `master` by comparing against a harcoded reference.
That is why it is expected to see a massive gate diff report on this PR.
The follow-up will revert the changes to accurately check against the
reference.

## Additional Context



## Documentation\*

Check one:
- [ ] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[For Experimental Features]** Documentation to be submitted in a
separate PR.

# PR Checklist\*

- [ ] I have tested the changes locally.
- [ ] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.
@TomAFrench TomAFrench merged commit 1931aaa into noir-lang:main Apr 25, 2024
3 checks passed
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