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(engine): fix calculation for labware height on modules #13364

Merged
merged 4 commits into from
Aug 25, 2023

Conversation

sanni-t
Copy link
Member

@sanni-t sanni-t commented Aug 22, 2023

Overview

I had forgotten to account for stacking overlap during calculation of the grip point when moving labware from/ to modules. This PR fixes that and adds a test to check for it.

Test Plan

Check that the gripping point of the same labware is same no matter if it's being moved to/ from:

  • a slot
  • a module with stacking overlap
  • a module without stacking overlap
  • a module with adapter
  • a module without adapter.

Changelog

  • updated the grip point calculation to fetch the labware origin coordinates from the existing geometry getter (instead of duplicating the logic, which I was wrongly doing before)
  • updated tests

Review requests

  • make sure I'm not missing anything else
  • @thassyopinto has tested on a mag block and confirmed that it fixed the issue being seen before. Testing on thermocycler was pending (did it complete yet?)

Risk assessment

None if done correctly.

@sanni-t sanni-t requested review from a team as code owners August 22, 2023 19:06
@sanni-t sanni-t requested review from brenthagen and removed request for a team August 22, 2023 19:06
@sanni-t sanni-t changed the base branch from chore_release-3.17.0 to chore_release-7.0.0 August 22, 2023 19:06
@codecov
Copy link

codecov bot commented Aug 22, 2023

Codecov Report

Merging #13364 (2aeedfe) into chore_release-7.0.0 (62063ca) will decrease coverage by 7.71%.
Report is 14 commits behind head on chore_release-7.0.0.
The diff coverage is n/a.

Impacted file tree graph

@@                   Coverage Diff                   @@
##           chore_release-7.0.0   #13364      +/-   ##
=======================================================
- Coverage                71.67%   63.96%   -7.71%     
=======================================================
  Files                     2427     1970     -457     
  Lines                    67641    43929   -23712     
  Branches                  7807     7771      -36     
=======================================================
- Hits                     48479    28100   -20379     
+ Misses                   17335    14002    -3333     
  Partials                  1827     1827              
Flag Coverage Δ
api ?
g-code-testing 96.44% <ø> (ø)
notify-server 89.13% <ø> (ø)
ot3-gravimetric-test ?
robot-server ?

Flags with carried forward coverage won't be shown. Click here to find out more.

see 461 files with indirect coverage changes

Copy link
Contributor

@SyntaxColoring SyntaxColoring left a comment

Choose a reason for hiding this comment

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

LGTM to merge, given the test approval. Thanks!

I am confused by the code, though. One docstring request that might help.

@sanni-t sanni-t merged commit 5e1b981 into chore_release-7.0.0 Aug 25, 2023
25 checks passed
@sanni-t sanni-t deleted the api-fix_labware_grip_point_calculation branch August 25, 2023 20:08
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