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

docs(api): more partial tip pickup docstrings #14132

Merged
merged 5 commits into from
Dec 8, 2023

Conversation

ecormany
Copy link
Contributor

@ecormany ecormany commented Dec 7, 2023

Overview

Expand on the initial docstring for configure_nozzle_layout (#13997) to include the new tip-tracking behavior (#14104).

Test Plan

Check the entries in the sandbox.

Changelog

  • Explained behavior of tip_rack parameter, with reference to same parameter of load_instrument, based on conversation with @sanni-t.
  • Expanded docstring for related active_channels property.

Review requests

This is how it works, yeah?

Risk assessment

nil, docstrings.

@ecormany ecormany added docs papi-v2 Python API V2 labels Dec 7, 2023
@ecormany ecormany requested a review from sanni-t December 7, 2023 17:10
@ecormany ecormany requested a review from a team as a code owner December 7, 2023 17:11
Comment on lines 1819 to 1821
*left-to-right* from a tip rack. Use ``"A12"`` to have the pipette use its
rightmost nozzles to pick up tips *right-to-left* from a tip rack.
:type start: str or ``None``
Copy link
Member

Choose a reason for hiding this comment

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

This is the opposite actually.. the rightmost nozzle column picks up tips left-to-right from the tiprack. Or am I not understanding this sentence corectly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you're right, it's wrong. I keep having to make hand motions every time I need to figure this out 😆

:param tip_racks: List of tip racks to use during this configuration.
:param tip_racks: Behaves the same as setting the ``tip_racks`` parameter of
:py:meth:`.load_instrument`. If not specified,
:py:obj:`.InstrumentContext.tip_racks` will be reset to ``None`` and you
Copy link
Member

Choose a reason for hiding this comment

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

tip_racks is reset to an empty list actually

Comment on lines 1828 to 1834
To keep the same list of tip racks when changing configurations, reference
the same instrument's existing ``tip_racks`` parameter::

pipette.configure_nozzle_layout(
style=COLUMN,
tip_racks=pipette.tip_racks
)
Copy link
Member

Choose a reason for hiding this comment

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

I feel like this would be better as part of the high level Opentrons docs than the API method docstrings because a) it's not critical info and b) it doesn't give you the full and probably more important details of why you actually shouldn't use the same tip racks when switching between the 96 channel's partial and full configurations.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, I just realized the note about 96 ch partial config tipracks already exists in this docstring. Then I'm cool keeping this here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nah, I think you're right. Too clever by half for docstrings.

@ecormany ecormany requested a review from sanni-t December 7, 2023 19:03
Copy link

codecov bot commented Dec 7, 2023

Codecov Report

Merging #14132 (c63a9af) into chore_release-7.1.0 (93c879a) will not change coverage.
Report is 11 commits behind head on chore_release-7.1.0.
The diff coverage is n/a.

Additional details and impacted files

Impacted file tree graph

@@                 Coverage Diff                  @@
##           chore_release-7.1.0   #14132   +/-   ##
====================================================
  Coverage                70.44%   70.44%           
====================================================
  Files                     2512     2512           
  Lines                    71201    71201           
  Branches                  8964     8964           
====================================================
  Hits                     50161    50161           
  Misses                   18849    18849           
  Partials                  2191     2191           
Flag Coverage Δ
g-code-testing 96.44% <ø> (ø)
notify-server 89.13% <ø> (ø)

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

Files Coverage Δ
...i/src/opentrons/protocol_api/instrument_context.py 89.40% <ø> (ø)

Copy link
Member

@sanni-t sanni-t left a comment

Choose a reason for hiding this comment

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

Changes look good!

There's just one thing to be aware of- we currently only (fully) support setting the rightmost column of the 96 channel for partial config. If you set the A1 as primary nozzle then your pipette will move correctly but there's no deck conflict checking or auto tip tracking for it yet. But it won't raise any errors either. So maybe we should put a note in there that currently only A12 as primary nozzle is fully supported and the others are use it at your own risk.

@ecormany
Copy link
Contributor Author

ecormany commented Dec 8, 2023

If you set the A1 as primary nozzle then your pipette will move correctly but there's no deck conflict checking

Talked with @anthonywilleee and we decided this is too risky to recommend, even with warnings. Gonna push another change to reflect that. I made a note on RTC-365 to come back to the A1 configuration later.

@ecormany
Copy link
Contributor Author

ecormany commented Dec 8, 2023

OK, I think this PR has finally settled into the same place we expect the feature to be at launch!

Copy link
Member

@sanni-t sanni-t left a comment

Choose a reason for hiding this comment

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

Nice! Changes since my last review look good!

@ecormany ecormany merged commit bfd1420 into chore_release-7.1.0 Dec 8, 2023
25 checks passed
@ecormany ecormany deleted the docstring-partial-tip_racks branch December 8, 2023 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs papi-v2 Python API V2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants