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(hardware-testing): grav script on ot3 #12302

Merged
merged 114 commits into from
Mar 17, 2023

Conversation

andySigler
Copy link
Contributor

Overview

Test Plan

Changelog

Review requests

Risk assessment

@codecov
Copy link

codecov bot commented Mar 16, 2023

Codecov Report

Merging #12302 (3fd06ff) into edge (b227887) will decrease coverage by 0.30%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             edge   #12302      +/-   ##
==========================================
- Coverage   73.69%   73.39%   -0.30%     
==========================================
  Files        2210     1480     -730     
  Lines       60971    48773   -12198     
  Branches     6280     2982    -3298     
==========================================
- Hits        44933    35798    -9135     
+ Misses      14588    12521    -2067     
+ Partials     1450      454     -996     
Flag Coverage Δ
hardware-testing ∅ <ø> (∅)
notify-server 89.13% <ø> (ø)

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

Impacted Files Coverage Δ
...pentrons/hardware_control/backends/ot3simulator.py 88.05% <ø> (ø)

... and 731 files with indirect coverage changes

Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

We need to split this PR up into multiple and figure something out for the merges that are baked in and inflate the diff.

I think some of these changes are because it merged in edge a couple times and got some other PRs or something?

Let's have separate PRs for

  • Changing blowout to take a volume
    • We need to rework this one to store shaft diameter in configuration rather than code
  • Changing the default motion parameters
  • Adding the temperature and humidity function
  • Changing the way homing works
  • Whatever happened with the change to cache instruments
  • Whatever's going on with fast home
  • The pipette configuration changes

We also need to somehow separate out

  • the tip overlap fix that was in a separate pr I think

@Laura-Danielle
Copy link
Contributor

@sfoster1 tip overlap fix is in the release branch right now.

@Laura-Danielle
Copy link
Contributor

@andySigler As discussed in our meeting earlier this week, I think I would prefer that any new functionality we want to support in the gravi test should live in a function inside the script. I would consider the blow out by volume to also be new functionality and we shouldn't be making any changes to the hardware controller for this.

We can try to keep parity by making sure those functions 1. use most of the underlying mechanisms in the hardware controller and 2. by talking with each other.

As we bring up that functionality in software, we can slowly switch the gravi script to use the hardware controller/protocol api again.

@andySigler andySigler force-pushed the feat-hardware-testing-grav-script-on-ot3 branch from e9fa398 to 8434e0b Compare March 16, 2023 15:48
@andySigler
Copy link
Contributor Author

Yes sry wasn't expecting to get eyes on this right away, should have labelled as draft, am going to remove all non hardware-testing changes for all the reasons you guys said and will ping you, thanks for taking a look!

@andySigler andySigler marked this pull request as draft March 16, 2023 17:33
@andySigler andySigler added the DO NOT MERGE Indicates a PR should not be merged, even if there's a shiny green merge button available label Mar 16, 2023
@andySigler andySigler force-pushed the feat-hardware-testing-grav-script-on-ot3 branch from 519a994 to ac217b3 Compare March 16, 2023 18:27
@andySigler andySigler force-pushed the feat-hardware-testing-grav-script-on-ot3 branch from ec43676 to 7a0d2c9 Compare March 16, 2023 19:55
@andySigler andySigler marked this pull request as ready for review March 17, 2023 16:50
@andySigler andySigler requested a review from sfoster1 March 17, 2023 16:50
@andySigler andySigler removed the DO NOT MERGE Indicates a PR should not be merged, even if there's a shiny green merge button available label Mar 17, 2023
Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

Okay this all makes a lot of sense to me now! Thanks for doing the changes.

Copy link
Contributor

@Laura-Danielle Laura-Danielle left a comment

Choose a reason for hiding this comment

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

This all looks fine to me.

@andySigler andySigler merged commit 055fafa into edge Mar 17, 2023
@andySigler andySigler deleted the feat-hardware-testing-grav-script-on-ot3 branch March 23, 2023 03:25
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.

4 participants