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

plate ID and empty metadata parsing #14

Merged
merged 3 commits into from
Feb 9, 2017
Merged

Conversation

qiyunzhu
Copy link
Collaborator

@qiyunzhu qiyunzhu commented Feb 8, 2017

Upon the requests from the wet lab crew, I modified two functions:

  1. "plate ID" matters, in addition to the "primer plate ID". The plate name is the first cell of a plate (upper left most). It can be any string. The "ID" is identified as the trailing numeric part of this string (if any). For example, "plate 15", "Plate15", "plate added two functions requested by wet lab crew #15", "Fecal plate 15" are all valid, and their IDs are all "15". This ID will be appended to the special samples (e.g., BLANK1.A1). This is to match the tradition of the wet lab.
  2. For a special sample type, if its metadata is not defined in the special sample definition file (empty list), then the original metadata as defined in the plate map will be used.
    Please let me know if you have any comments. Thank you!

@wasade @mortonjt @josenavas @antgonza @sjanssen2

@coveralls
Copy link

coveralls commented Feb 8, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling f4897f6 on qiyunzhu:validate into 2a90655 on biocore:master.

Copy link
Collaborator

@mortonjt mortonjt left a comment

Choose a reason for hiding this comment

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

Minor comments about syntax.

@@ -110,7 +117,7 @@ def plate_mapper(input_f, barseq_f, output_f, names_f=None, special_f=None):
for line in special_f:
line = line.rstrip()
l = line.split('\t')
if len(l) < 4:
if len(l) < 3:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a comment about what this number is supposed to be?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure! Will do. @mortonjt

@@ -127,19 +134,22 @@ def plate_mapper(input_f, barseq_f, output_f, names_f=None, special_f=None):
# [ barcode sequence, linker primer sequence, primer plate #, well ID ]
sample, property = '', []
if plate in plates:
id = primer2plate[plate]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not recommended, since id is a reserved keyword. One option is to rename this variable to id_

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How about "pid" (plate ID)?

@@ -127,19 +134,22 @@ def plate_mapper(input_f, barseq_f, output_f, names_f=None, special_f=None):
# [ barcode sequence, linker primer sequence, primer plate #, well ID ]
sample, property = '', []
Copy link
Collaborator

Choose a reason for hiding this comment

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

Too bad we didn't catch this last time. property is also a reserved keyword. This should be replace with something like property_

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How about "metadata"?

sample = '%s%s.%s' % (specs[sample]['name'], plate, well)
if specs[sample]['property']:
# replace property if available
property = specs[sample]['property']
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here

@@ -127,19 +134,22 @@ def plate_mapper(input_f, barseq_f, output_f, names_f=None, special_f=None):
# [ barcode sequence, linker primer sequence, primer plate #, well ID ]
sample, property = '', []
if plate in plates:
id = primer2plate[plate]
if well in plates[plate]:
sample, property = plates[plate][well], properties[plate]
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here

if specs[sample]['property']:
# replace property if available
property = specs[sample]['property']
sample = '%s%s.%s' % (specs[sample]['name'], id, well)
else:
# normal sample name
samples.append(sample)
elif '' in specs:
# empty well (if defined as a special sample)
property = specs['']['property']
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here

@mortonjt
Copy link
Collaborator

mortonjt commented Feb 8, 2017 via email

@mortonjt
Copy link
Collaborator

mortonjt commented Feb 8, 2017 via email

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 7eb6a98 on qiyunzhu:validate into 2a90655 on biocore:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 7eb6a98 on qiyunzhu:validate into 2a90655 on biocore:master.

Copy link
Member

@wasade wasade left a comment

Choose a reason for hiding this comment

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

Only one minor comment

@@ -73,16 +75,21 @@ def plate_mapper(input_f, barseq_f, output_f, names_f=None, special_f=None):
elif int(v) != cols+1:
raise ValueError('Error: column headers are not '
'incremental integers.')
# get plate ID (trailing numeric part)
for i in reversed(l[0]):
Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure this look can be replaced with a regex, but at the sametime, I'm not very concerned about the logic

Copy link
Collaborator Author

@qiyunzhu qiyunzhu Feb 8, 2017

Choose a reason for hiding this comment

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

@wasade Thanks for the comment! I love regex because I was a Perl person. Here I chose this way so that I don't have to import re. Let's leave it as is?

Copy link
Collaborator

Choose a reason for hiding this comment

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

re comes bundled with the python standard library - so importing it shouldn't be a problem.

@qiyunzhu
Copy link
Collaborator Author

qiyunzhu commented Feb 9, 2017

@wasade @mortonjt Can we merge now?

@qiyunzhu
Copy link
Collaborator Author

qiyunzhu commented Feb 9, 2017

Regex added. @mortonjt @wasade

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 0f43046 on qiyunzhu:validate into 2a90655 on biocore:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 0f43046 on qiyunzhu:validate into 2a90655 on biocore:master.

@mortonjt mortonjt merged commit e8d9424 into biocore:master Feb 9, 2017
@qiyunzhu qiyunzhu deleted the validate branch August 5, 2017 20:28
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.

None yet

4 participants