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

Timeout GreedyPauliSimp #1684

Merged
merged 23 commits into from
Nov 19, 2024
Merged

Timeout GreedyPauliSimp #1684

merged 23 commits into from
Nov 19, 2024

Conversation

sjdilkes
Copy link
Contributor

@sjdilkes sjdilkes commented Nov 18, 2024

Add trials option to GreedyPauliSimp, running multiple attempts at GreedyPauliSimp and returning the "smallest" circuit.

Checklist

  • I have performed a self-review of my code.
  • I have commented hard-to-understand parts of my code.
  • I have made corresponding changes to the public API documentation.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have updated the changelog with any user-facing changes.

@sjdilkes sjdilkes marked this pull request as ready for review November 19, 2024 09:31
Comment on lines 835 to 837
std::queue<
std::pair<std::future<Circuit>, std::shared_ptr<std::atomic<bool>>>>
all_threads;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not anymore!

c.decompose_boxes_recursively();
circuits.push_back(c);
} else {
// If the thread is not ready, move it to the back of the queue
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you elaborate on this comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, it's actually outdated from an earlier version with the multithreading - I can remove it

"\n:param timeout: Sets maximum out of time spent finding solution."
"\n:param thread_timeout: Sets maximum out of time spent finding a "
"single solution in one thread."
"\n:param trials: Sets maximum number of found solutions."
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe explain what qualifies as the best solution?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah good point, have done

Copy link
Contributor

@yao-cqc yao-cqc left a comment

Choose a reason for hiding this comment

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

Only one issue with the docs, and we also need to update the stubs.

Comment on lines 456 to 460
"\n:param trials: Sets maximum number of found solutions. The "
"circuit "
"smallest circuit is returned, priorising the number of 2qb-gates, "
"then "
"gates, then depth."
Copy link
Contributor

Choose a reason for hiding this comment

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

Also add this to passes.cpp?
The lines are quite jumbled. The circuit smallest circuit is returned should be The smallest circuit is returned .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done!

@yao-cqc
Copy link
Contributor

yao-cqc commented Nov 19, 2024

Still need to update the stubs

@sjdilkes
Copy link
Contributor Author

Still need to update the stubs

Oops done, sorry forgot to push

@sjdilkes sjdilkes merged commit ef27760 into main Nov 19, 2024
32 checks passed
@sjdilkes sjdilkes deleted the single-thread-GPO branch November 19, 2024 18:18
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