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

bugfix/retry_result: 0.0 is False #350

Merged
merged 2 commits into from
Oct 28, 2024
Merged

bugfix/retry_result: 0.0 is False #350

merged 2 commits into from
Oct 28, 2024

Conversation

rogerlz
Copy link
Contributor

@rogerlz rogerlz commented Aug 20, 2024

_Enter a good description of whats being changed and WHY

Checklist

  • pr title makes sense
  • squashed to 1 commit
  • added a test case if possible
  • if new feature, added to the readme
  • ci is happy and green

@rogerlz rogerlz requested a review from a team as a code owner August 20, 2024 23:36
@rogerlz rogerlz marked this pull request as draft August 21, 2024 00:47
@ThaoDChan
Copy link
Contributor

Hi,

I encountered the same issue that user Turpinator described on Armchair Discord.

I tested your PR, and it worked, but I observed that z_tilt.applied was set to True after the second probing run out of a total of three runs.

After looking into the code, I believe the condition in check_retry_result can prematurely set applied to True when retry_result is a small positive float:
int(retry_result) == 0 evaluates to True for any retry_result between 0.0 and 1.0, even if error > retry_tolerance
As a result, z_tilt.applied becomes True before probing is completed.

Here's the solution I came up with:

def check_retry_result(self, retry_result):
 if (
     (isinstance(retry_result, str) and retry_result == "done")
     or (isinstance(retry_result, float) and retry_result == 0.0)
 ):
     self.applied = True
 return retry_result

This change ensures that applied is only set to True when retry_result is exactly 0.0, avoiding false positives caused by small float values.

If you have any questions or need further clarification, feel free to reach out.
My Discord username is Kingn00dles.

@turpinator
Copy link

I tried the code from @ThaoDChan and QGL is applied as expected.
After a single successful iteration at QGL, it applies.
After multiple iterations at QGL, it applies.
After a single 'non pass' iteration, QGL does not apply (expected).

@rogerlz
Copy link
Contributor Author

rogerlz commented Oct 23, 2024

thanks for the patch!

@rogerlz rogerlz marked this pull request as ready for review October 23, 2024 08:55
@rogerlz rogerlz merged commit aa89886 into master Oct 28, 2024
2 checks passed
@rogerlz rogerlz deleted the bugfix/retry_result branch October 28, 2024 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants