-
Notifications
You must be signed in to change notification settings - Fork 33
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
y24-336 - BGE Request Types and Submissions #4511
y24-336 - BGE Request Types and Submissions #4511
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4511 +/- ##
===========================================
+ Coverage 86.48% 89.21% +2.73%
===========================================
Files 1392 1392
Lines 29830 29931 +101
===========================================
+ Hits 25797 26703 +906
+ Misses 4033 3228 -805 ☔ View full report in Codecov by Sentry. |
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.
Apologies story was slightly incorrect regarding the ISC part. You may need to add BGE Cherrypicked, BGE Lib XP2 and BGE Lib PCR XP to config/default_records/plate_purposes/* for them to show up as acceptable purposes.
config/default_records/request_types/020_limber_high_throughput_bge.wip.yml
Outdated
Show resolved
Hide resolved
config/default_records/request_types/020_limber_high_throughput_bge.wip.yml
Outdated
Show resolved
Hide resolved
config/default_records/request_types/020_limber_high_throughput_bge.wip.yml
Outdated
Show resolved
Hide resolved
BGE Lib XP2: | ||
type: PlatePurpose::Input | ||
target_type: Plate | ||
stock_plate: true |
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.
I don't think BGE Lib XP2 or BGE Lib PCR XP are stock_plate's. @KatyTaylor might know a bit more about these attributes.
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.
BGE Cherrypick should be an 'input' plate (as you have done) because it's the first one in the pipeline. I'm not sure whether it's necessary for Lib XP2 and Lib PCR XP or not. If it was me I'd start of without and add it back in later if necessary.
I don't think any of them need to be 'stock' plates. A plate needs to be a 'stock' if it's used in a sample manifest - none of these are. And I don't think they would be considered stock plates by the lab as they're not the ones that came directly from the customer.
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.
LB Cherrypick and PF Cherrypicked (that BGE Cherrypick is modelled off) are both set as stock_plate: true.
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.
Nice looks good, just unsure about some of the plate purposes data.
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.
Left some questions and things to check
size: 96 | ||
BGE Lib XP2: | ||
target_type: Plate | ||
default_state: passed |
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.
don't think default state is passed on these XP plates. maybe for the cherrypick as its an input plate
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.
although I now see there are submissions for each of these plates so not sure
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.
Used here -
ALL_STATES.detect { |s| unique_states.include?(s) } || default_state || 'unknown' |
So when trying to determine a plate's state, it looks first at the state of the transfer requests. If it can't determine the state from the transfer requests (there are 0 or >1 that aren't in the expected list), then it'll use the default state.
So I suppose for plates that are the first in a pipeline, they won't have transfer requests coming into them, so it'll use the default state. That's the cherrypick plate but not the other two.
Got a meeting so can't continue looking into it now, but guess it's also relevant to think about how the submission affects the state (can't remember where that code is).
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.
I think @KatyTaylor is correct. I checked the other purpose configs in SS, and only the BGE Cherrypick should have the passed
default state.
target_type: Plate | ||
default_state: passed | ||
stock_plate: false | ||
cherrypickable_target: true |
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.
and only the cherrypick plate should have the cherrypickable_target true unless something special is happening here
acceptable_purposes: | ||
- BGE Lib PCR XP | ||
library_types: | ||
- BGE |
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.
Is library type 'BGE' for all 3 of these despite them being different library preps? Seems wrong if they want to analyse the sequencing results differently. Maybe check with users.
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.
BGE Library type is already used across the pipelines (and submissions) that area already in place (that these are modelled off). It is essentially all part of one bigger pipeline where all the samples are recombined and sequenced together.
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.
Looks good, just one comment about whether the BGE Cherrypick needs to be a stock_plate.
@KatyTaylor @andrewsparkes Any objections to merging? I think this is fine to merge now because its wip'd and unusable until the pipeline is made, in which any additional changes can be made then. |
@BenTopping fine by me! |
Closes #4402
Changes proposed in this pull request
Instructions for Reviewers
[All PRs] - Confirm PR template filled
[Feature Branches] - Review code
[Production Merges to
main
]- Check story numbers included
- Check for debug code
- Check version