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.

Sign up for GitHub

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

Handle some floating point edge cases in truncation #65

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+4 −2
Open

Handle some floating point edge cases in truncation #65

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d4df90a
Handle some floating point edge cases in truncation
caleb-johnson Mar 5, 2025
8871520
style
caleb-johnson Mar 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions qiskit_addon_obp/utils/truncating.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,9 @@ def truncate_binary_search(
lower_error = 0.0

# binary search for a cutoff threshold
while ((upper_threshold - lower_threshold) > 1e-10) and (upper_error != lower_error):
while ((upper_threshold - lower_threshold) > 1e-10) and not (
np.isclose(upper_error, lower_error)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

default atol is 1e-8, @BryceFuller , @mrossinek . Is that fine? We don't take a tolerance to this func as input

Copy link
Member

Choose a reason for hiding this comment

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

It seems a bit arbitrary to me, to mix the 1e-10 threshold in the first statement with an atol=1e-8 and rtol=1e-5 in np.isclose. If we could unify that and maybe even expose it as a setting that would be better imo

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@BryceFuller and I are also trying to determine if we need both while conditionals here. Need to think on it a bit before finishing this up

):
mid_threshold = (upper_threshold + lower_threshold) / 2
# PERF: the boolean indexing here will need to check every element in the array at every
# iteration of this loop. We can improve the performance by performing successive
Expand All @@ -186,7 +188,7 @@ def truncate_binary_search(
upper_error = mid_error

# delete according to lower_threshold
to_keep = abscs > lower_threshold
to_keep = abscs >= lower_threshold

return (
SparsePauliOp(observable.paulis[to_keep], observable.coeffs[to_keep]),
Expand Down