-
Notifications
You must be signed in to change notification settings - Fork 178
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(api): Automatic tip tracking index out of range fix #15135
fix(api): Automatic tip tracking index out of range fix #15135
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logically this makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes look like they'll solve the issue with tip tracking. Made a couple suggestions to potentially simplify these changes. If time allows I think a further refactor could simplify a lot of this logic, as well as going further since our whole tip tracking behavior is very spaghetti code to begin with.
@@ -312,7 +312,9 @@ def _cluster_search_A1(active_columns: int, active_rows: int) -> Optional[str]: | |||
if critical_row + active_rows < len(columns[0]): | |||
critical_row = critical_row + active_rows | |||
else: | |||
critical_column = critical_column + 1 | |||
critical_column += 1 | |||
if critical_column >= len(columns): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This if statement could probably be removed and the while check be changed to while critical_column < len(columns) if I understand this correctly, since we're not doing any indexing immediately after this.
@@ -336,7 +338,9 @@ def _cluster_search_A12(active_columns: int, active_rows: int) -> Optional[str]: | |||
if critical_row + active_rows < len(columns[0]): | |||
critical_row = critical_row + active_rows | |||
else: | |||
critical_column = critical_column - 1 | |||
critical_column -= 1 | |||
if critical_column < 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing as above, except I don't think you'd need to change anything in the while conditional
…mption is occurring correctly in all directions
Overview
Addresses RQA-2700
Automatic tip tracking when iterating through multiple tip racks did not take into account progression range across those tipracks resulting in the potential for an index out of range error to manifest under certain conditions. These changes seek to fix that.
Test Plan
Ensure that API 2.18 protocols consume tipracks without encountering an index out of range error manifesting as an out of tips error when iterating through multiple tipracks using tip-cluster based automatic tip tracking. Protocols such as the one below should execute without issue.
Changelog
Review requests
Do these changes appear comprehensive enough given existing safegaurds within the tiprack iteration logic?
Risk assessment
Low, fixes oversight.