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 bug for max_t_factories constraint #1792

Merged
merged 1 commit into from
Jul 25, 2024
Merged

Conversation

msoeken
Copy link
Member

@msoeken msoeken commented Jul 25, 2024

A computation to determine the number of T factories was numerically imprecise leading to results in which the constraint was not satisfied.

The computation has been fixed. Also the chemistry RE benchmark has been updated for which this problem has been discovered in the first place. Corresponding test cases for the chemistry RE benchmark are updated accordingly.

Copy link

Benchmark for 919ba5c

Click to view benchmark
Test Base PR %
Array append evaluation 346.4±2.68µs 350.0±23.44µs +1.04%
Array literal evaluation 174.9±4.32µs 180.9±9.44µs +3.43%
Array update evaluation 415.1±6.29µs 416.1±11.50µs +0.24%
Core + Standard library compilation 24.5±0.88ms 23.7±0.94ms -3.27%
Deutsch-Jozsa evaluation 4.9±0.07ms 5.0±0.20ms +2.04%
Large file parity evaluation 34.2±0.61ms 34.1±0.37ms -0.29%
Large input file compilation 14.1±0.66ms 14.5±0.77ms +2.84%
Large input file compilation (interpreter) 56.9±3.71ms 56.0±2.38ms -1.58%
Large nested iteration 32.2±0.26ms 32.4±1.94ms +0.62%
Perform Runtime Capabilities Analysis (RCA) on Deutsch-Jozsa sample 1622.3±118.73µs 1628.8±169.84µs +0.40%
Perform Runtime Capabilities Analysis (RCA) on large file sample 8.1±0.17ms 8.3±0.21ms +2.47%
Perform Runtime Capabilities Analysis (RCA) on teleport sample 1451.7±59.68µs 1463.1±134.09µs +0.79%
Perform Runtime Capabilities Analysis (RCA) on the core and std libraries 28.1±0.66ms 28.4±0.63ms +1.07%
Teleport evaluation 90.6±3.23µs 91.0±3.29µs +0.44%

@swernli
Copy link
Collaborator

swernli commented Jul 25, 2024

@ivanbasov Can you take a look too? The Rust code seems fine to me, but given how much the test case and the numbers validated changed I'm not sure how to verify the results are reasonable.

@msoeken msoeken added this pull request to the merge queue Jul 25, 2024
Merged via the queue into main with commit 8756d0d Jul 25, 2024
19 checks passed
@msoeken msoeken deleted the msoeken/re-num-factories branch July 25, 2024 19:41
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.

3 participants