-
Notifications
You must be signed in to change notification settings - Fork 88
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
Gregor metadata airtable update #3556
Conversation
…roadinstitute/seqr into gregor-metadata-airtable-update
content_rows = [[row.get(key) or blank_value for key in header] for row in rows] | ||
content += '\n'.join([ | ||
DELIMITERS[file_format].join(row) for row in content_rows | ||
content = '\n'.join([ |
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.
fixes a bug when the table is empty
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.
Add a test for an empty table?
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.
out of scope, I just happened to notice it here but this endpoint can't return empty tables
seqr/views/apis/report_api.py
Outdated
READ_RNA_TABLE_COLUMNS.insert(10, 'gene_annotation_details') | ||
READ_RNA_TABLE_COLUMNS.insert(13, 'alignment_postprocessing') |
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.
The number is hard to maintain. Can we use indexOf()
to get relative positions?
RNA_ONLY = EXPERIMENT_RNA_TABLE_AIRTABLE_FIELDS + READ_RNA_TABLE_AIRTABLE_FIELDS + ['reference_assembly_uri'] | ||
DATA_TYPE_OMIT = { | ||
'wgs': ['targeted_regions_method'] + RNA_ONLY, 'wes': RNA_ONLY, 'rna': [ | ||
'targeted_regions_method', 'target_insert_size', 'mean_coverage', 'aligned_dna_short_read_file', | ||
'aligned_dna_short_read_index_file', | ||
], | ||
} | ||
NO_DATA_TYPE_FIELDS = { | ||
'targeted_region_bed_file', 'reference_assembly', 'analysis_details', 'percent_rRNA', 'percent_mRNA', | ||
'alignment_software_dna', |
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.
It is worth defining constants for these field names.
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.
Why? We don't for the other 7 tables already in this report, why would we do it for this new one
individual_data_types = defaultdict(set) | ||
for individual_db_id, sample_type in sample_types: | ||
individual_data_types[individual_db_id].add(sample_type) | ||
individuals = Individual.objects.filter(id__in=individual_data_types).prefetch_related( |
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.
Interestingly, we don't have to use individual_data_types.keys()
for Individual.objects.filter(id__in=individual_data_types)
.
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.
thats just how iterating a dictionary in python3 works
content_rows = [[row.get(key) or blank_value for key in header] for row in rows] | ||
content += '\n'.join([ | ||
DELIMITERS[file_format].join(row) for row in content_rows | ||
content = '\n'.join([ |
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.
Add a test for an empty table?
No description provided.