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

specify counts columns in manual_drops #46

Merged
merged 3 commits into from
Apr 24, 2024
Merged

Conversation

ckikawa
Copy link
Collaborator

@ckikawa ckikawa commented Apr 23, 2024

This pull request should resolve #45 by adding additional elif statements to a cell of process_plate.ipynb where manual_drops are removed from the counts dataframe. I tested this with multiple wells on some recent H3N2 ngs-neut data.

As is, single wells/barcodes cannot be specified in lists (as indicated in README). They must be added as individual lines with dashes. If this is expected behavior, ignore this comment. Otherwise, I can add this detail to README?

@ckikawa ckikawa requested a review from jbloom April 23, 2024 23:24
@anloes
Copy link
Collaborator

anloes commented Apr 23, 2024

I think we should add instructions to the README.md, if was not immediately obvious that this is what should be done, an
example would work.

Copy link
Member

@jbloom jbloom left a comment

Choose a reason for hiding this comment

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

@ckikawa, can you just update this in the CHANGELOG too. As I understand it, this is just a bug fix so that can be a minor version (3.1.1) as no new functionality was added, it just makes something that already is supposed to happen work correctly.

Just briefly explain what is fixed and link to the issue, as in other CHANGELOG entries.

I don't quite understand @anloes's comment as I don't think anything needs to be added to README as this is just fixing a bug that prevented previously documented functionality from actually working correctly, right? Can you clarify that is correct with her?

@anloes
Copy link
Collaborator

anloes commented Apr 23, 2024

Yes, you are correct that this was a bug fix. However, there is not currently an example of how a single well or a barcode should be listed for exclusion. It was unclear if they should be in brackets around the entry or if these could just be listed as strings. I would recommend we update the README.md section on manual drops to show an example of each, i.e.:

manual_drops:
barcode_serum_replicates:
- [AGTCCTATCCTCAAAT, M099d0]
wells:
- A1

@jbloom
Copy link
Member

jbloom commented Apr 23, 2024

It should just be a list, which can be specified in either of two ways in YAML:

wells: [A1, A2]

or

wells:
  - A1
  - A2

It says this verbally in README, but you can add in example if you want @ckikawa

@ckikawa
Copy link
Collaborator Author

ckikawa commented Apr 23, 2024

@jbloom, I see. I think this was just a YAML formatting quirk that I wasn't familiar with. I was trying (and failing) with:

wells:
  - [A1, A2]

I've updated the CHANGELOG but I'm not sure an example in README is necessary.

@anloes anloes assigned anloes and unassigned anloes Apr 23, 2024
@jbloom jbloom merged commit 98b8d10 into main Apr 24, 2024
1 check passed
@jbloom jbloom deleted the 45_change_manual_drops_keys branch April 24, 2024 01:45
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.

plate-level manual drops with 'wells' causes assertion error
3 participants