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

feat(api): remove use of LPC offsets during labware movement #13144

Merged
merged 2 commits into from
Jul 21, 2023

Conversation

sanni-t
Copy link
Member

@sanni-t sanni-t commented Jul 20, 2023

Overview

Closes RLAB-237

From hardware testing it has been found that adding LPC offsets during labware movement with gripper makes labware pick ups and drops unreliable. But having the pickUp & drop offsets is useful for fine tuning the default positions of labware pickUp & drop. So this PR removes the use of just LPC offsets from API and engine's labware movement commands.

Helps reduce change diff for RLAB-295.

Test Plan

Just unit tests and linting should suffice since this is removing a functionality that neither science/beta testers not hardware testing is using. Still, a quick test of labware movement on the flex would be helpful in making sure everything still works before making the larger changes of RLAB-295.

Changelog

  • removed use_pick_up_location_lpc_offset and use_drop_location_lpc_offset from protocol_context.move_labware args
  • removed usePickUpLocationLpcOffset and useDropLocationLpcOffset from moveLabware engine command
  • renamed any args/ classes that were called "experimental"
  • added a missing sync_client test

Review requests

  • It's a very straightforward removal of args & params so should be an easy review

Risk assessment

Low. Just a risk about 'whether the lpc offsets could ever be useful?'. But we can always put them back in if that happens.

@sanni-t sanni-t requested review from a team as code owners July 20, 2023 21:36
@codecov
Copy link

codecov bot commented Jul 20, 2023

Codecov Report

Merging #13144 (f503a71) into edge (fc2ffdb) will increase coverage by 0.47%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #13144      +/-   ##
==========================================
+ Coverage   72.08%   72.55%   +0.47%     
==========================================
  Files        1530     2390     +860     
  Lines       50741    65933   +15192     
  Branches     3144     7306    +4162     
==========================================
+ Hits        36576    47838   +11262     
- Misses      13657    16350    +2693     
- Partials      508     1745    +1237     
Flag Coverage Δ
app 71.26% <ø> (+27.32%) ⬆️
g-code-testing 96.44% <ø> (ø)
notify-server 89.13% <ø> (ø)
protocol-designer 47.29% <ø> (ø)
shared-data 78.40% <ø> (-0.95%) ⬇️
step-generation 87.17% <ø> (ø)

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

Impacted Files Coverage Δ
api/src/opentrons/protocol_api/protocol_context.py 91.66% <ø> (-0.14%) ⬇️
...c/opentrons/protocol_engine/clients/sync_client.py 100.00% <ø> (ø)
api/src/opentrons/protocol_engine/types.py 97.65% <ø> (-0.06%) ⬇️

... and 869 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.

Code LGTM, thanks! Nice catch on the missing SyncClient test.

api/src/opentrons/protocol_engine/types.py Outdated Show resolved Hide resolved
@sanni-t sanni-t merged commit e1da55c into edge Jul 21, 2023
@sanni-t sanni-t deleted the api-remove_lpc_offsets_from_labware_movement_params branch July 30, 2024 16:20
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