Skip to content

Comments

Christian cbs prioritization#275

Open
123chrisc wants to merge 2 commits intomasterfrom
christian-cbs-prioritization
Open

Christian cbs prioritization#275
123chrisc wants to merge 2 commits intomasterfrom
christian-cbs-prioritization

Conversation

@123chrisc
Copy link

Briefly describe the feature or bug that this PR addresses.

Former estimate prioritization did not account for antibody target hierarchy. I have added the hierarchy as per 2021.07.06 LSR protocol to address pooling issues in CBS report.

Please link the Airtable ticket associated with this PR.

N/A

Describe the steps you took to test the feature/bugfix introduced by this PR.

Reviewed LSR protocol. Added relevant prioritization code.

Does any infrastructure work need to be done before this PR can be pushed to production?

I don't think so

@rahularoradfs
Copy link
Contributor

@123chrisc nice code!

lambda estimate: len(estimate['antibody_target']) > 1, # Multiple AB targets - is this guaranteed to be multiple targets including both a spike target and a nucleocapsid target, or could it also include multiple nucleocapsid targets? Think through all edge cases

Otherwise logic seems sound

Will leave proper review to a dev team member!

@rahularoradfs
Copy link
Contributor

Can we get this merged @123chrisc @simonarocco/

@123chrisc
Copy link
Author

@rahularoradfs Since its on the estimate sheet, multiple AB targets will only be flagged if multiple unique AB targets are selected! So multiple tests that solely target nucleocapsid wouldn't appear, but different combinations with nucleocapsid (e.g. nucleocapsid + whole membrane) could pop-up.

I think everything should be good otherwise!

@simonarocco
Copy link
Contributor

Can we get this merged @123chrisc @simonarocco/

@123chrisc we can merge, but we are getting rid of estimate prioritization anyways! ;) doesnt really matter

@rahularoradfs
Copy link
Contributor

rahularoradfs commented Nov 9, 2021 via email

Copy link
Contributor

@simonarocco simonarocco left a comment

Choose a reason for hiding this comment

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

lgtm, just one small comment

'column_names',
'summary_function'])

#test
Copy link
Contributor

Choose a reason for hiding this comment

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

do you need this comment @123chrisc ?

Copy link
Author

Choose a reason for hiding this comment

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

No I don't think so! Was playing around with using doing merge requests from terminal instead of GitHub desktop. Is this comment still being actually displayed? I thought I had removed it

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