External project details readsets tab - #1181
Conversation
|
You might have forgotten to add a dependency to package.json Fixed with |
| ACTIVE_PARENT_PROJECT_READSET_FILTERS = { | ||
| "deleted": False, | ||
| "dataset__deleted": False, | ||
| "dataset__project__deleted": False, | ||
| "dataset__project__parent_project__deleted": False, | ||
| "dataset__experiment_run__deleted": False, | ||
| "dataset__experiment_run__run_type__deleted": False, | ||
| "dataset__experiment_run__run_type__platform__deleted": False, | ||
| "derived_sample__deleted": False, | ||
| "derived_sample__biosample__deleted": False, | ||
| "derived_sample__biosample__individual__deleted": False, | ||
| "derived_sample__library__deleted": False, | ||
| "derived_sample__library__library_type__deleted": False, | ||
| "derived_sample__derived_by_samples__deleted": False, | ||
| "derived_sample__derived_by_samples__sample__deleted": False, | ||
| "derived_sample__derived_by_samples__sample__container__deleted": False, | ||
| } |
There was a problem hiding this comment.
I would call this DEFAULT_PARENT_PROJECT_READSET_FILTERS
There was a problem hiding this comment.
And I also believe this should be moved inside get_parent_project_readsets_queryset
There was a problem hiding this comment.
A small note : The deleted flag is typically used to save a last version in reversion before deleting an object for good. This means there should be no "deleted" objects in the DB except in the reversion tables. These filter will likely slow the query without much to offer.
There was a problem hiding this comment.
Thanks. I removed the redundant deleted=False filters ... Fixed.
| PARENT_PROJECT_READSET_ORDERING = [ | ||
| "dataset__experiment_run__start_date", | ||
| "dataset__experiment_run__name", | ||
| "dataset__lane", | ||
| "id", | ||
| ] | ||
|
|
||
| PARENT_PROJECT_READSET_VALUE_FIELDS = [ | ||
| "id", | ||
| "name", | ||
| "average_quality", | ||
| "pf_reads_aligned", | ||
| "duplicate_aligned", | ||
| "readset_file_paths", | ||
| "readset_file_sizes", | ||
| "barcodes", | ||
| "validation_status", | ||
| ] | ||
|
|
||
| PARENT_PROJECT_READSET_VALUE_ALIASES = { | ||
| "readset_sample_name": F("sample_name"), | ||
| "external_id": F("dataset__project__parent_project__external_id"), | ||
| "run_name": F("dataset__experiment_run__name"), | ||
| "lane": F("dataset__lane"), | ||
| "reference_genome_id": F("derived_sample__biosample__individual__reference_genome_id"), | ||
| "sequencing_index_name": F("derived_sample__library__index__name"), | ||
| "run_start_date": F("dataset__experiment_run__start_date"), | ||
| "alias": F("derived_sample__biosample__alias"), | ||
| "cohort": F("derived_sample__biosample__individual__cohort"), | ||
| "library_type": F("derived_sample__library__library_type__name"), | ||
| "number_of_reads": F("production_data__reads"), | ||
| } |
There was a problem hiding this comment.
These should be moved inside get_parent_project_readsets_queryset unless they have a use outside the function.
There was a problem hiding this comment.
Having codes in backend/fms_core/queries/ is stepping outside the usual pattern for creating queryset in Freezeman.
There was a problem hiding this comment.
They are normally achieved with services or viewsets or filters.py or serializers.py
There was a problem hiding this comment.
Nafiz is right, but i honestly find your solution tempting ... The main downside i find to it is that these queries are likely not going to be reused outside a specific viewset and in the end it would just force code reader to move back and forth between code sections (one of the things i dislike the most about our frontend ;). I would tend to recommend we move these query within the viewset itself.
There was a problem hiding this comment.
Agreed. I moved the parent project readsets queryset logic into ParentProjectViewSet as a private helper, since it is specific to this action.
There was a problem hiding this comment.
Brand new submodules shouldn't be introduced without master's permission @UlysseFG's agreement.
There was a problem hiding this comment.
Thanks ..
Addressed by removing the new queries package and moving the action-specific logic into ParentProjectViewSet.
| const total = items.length || 1 | ||
|
|
There was a problem hiding this comment.
It's strange to always be needing at least total of 1.
| return { | ||
| complete: Math.round((complete / total) * 100), | ||
| incomplete: Math.round((incomplete / total) * 100), | ||
| completeCount: complete, | ||
| incompleteCount: incomplete, | ||
| } |
There was a problem hiding this comment.
Ah, that's why you wanted total of at least 1. I feel like it would be better instead to do something like
complete: Math.round((complete / Math.max(total, 1)) * 100),
incomplete: Math.round((incomplete / Math.max(total, 1)) * 100),or
complete: total == 0 ? 0 : Math.round((complete / total) * 100),
incomplete: total == 0 ? 0 : Math.round((incomplete / total) * 100),There was a problem hiding this comment.
I support this approach it is less risky if we ever decide to use total elsewhere. 0 is also a very valid value when not used as a denominator... All projects will have 0 readset at some point.
There was a problem hiding this comment.
Updated to keep the actual total and protect the percentage calculations when total is 0.
| const total = readsets.length || 1 | ||
|
|
There was a problem hiding this comment.
Similar suggestion I've made for frontend/src/components/projectOverview/ExternalIDReadSetDashboard.tsx
There was a problem hiding this comment.
This is corrections that were supposed to be done on the PR that introduced those code.
|
|
||
| const projectOverviewReadsetColumns = getProjectOverviewReadsetColumns(libraryTypeFilters) | ||
|
|
||
| if (isLoading) { |
There was a problem hiding this comment.
getProjectOverviewReadsetColumns result should be memoized.
| totalReadsets: readsets.length, | ||
| totalReads: readsets.reduce((sum, x) => sum + Number(x.number_of_reads || 0), 0), | ||
| totalRuns: new Set(readsets.map((x) => x.run_name)).size, | ||
| totalSamples: new Set(readsets.map((x) => x.readset_sample_name)).size, |
There was a problem hiding this comment.
sample name is not unique. You should not trust it for counting independant samples. You would be better to count biosample_id. It is likely to be mostly the same but there might be case where this will make a difference.
There was a problem hiding this comment.
Thanks a lot ... This is fixed ....
| totalReads: readsets.reduce((sum, x) => sum + Number(x.number_of_reads || 0), 0), | ||
| totalRuns: new Set(readsets.map((x) => x.run_name)).size, | ||
| totalSamples: new Set(readsets.map((x) => x.readset_sample_name)).size, | ||
| totalCohorts: new Set(readsets.map((x) => x.cohort)).size, |
There was a problem hiding this comment.
This is a metric that will likely not be very useful. Projects usually don't have many cohorts and the number of cohort is not an indicator for much of anything. You can leave it, but keep in mind if you ever need to free some space on the UI that this is a good candidate for replacement.
| "files__file_path", | ||
| filter=Q(files__deleted=False),distinct=True,default=Value([]), | ||
| ), | ||
| readset_file_sizes=ArrayAgg( |
There was a problem hiding this comment.
It would be better to combine each file with its size in a single cell instead of having to rely on matching position in a list. I admit this would not render well in a CSV but the list are likely to break the regularity of the structure.
There was a problem hiding this comment.
Updated to return and render readset files as a single list of objects containing both file_path and size,
| filter=Q(files__deleted=False),distinct=True,default=Value([]), | ||
| ), | ||
| barcodes=ArrayAgg( | ||
| "derived_sample__derived_by_samples__sample__container__barcode", |
There was a problem hiding this comment.
I think there might still be some confusion between sample barcodes and container barcodes ... I would be very surprised we have to report container barcode anywhere in the project overview unless it is run container barcodes... At the readset level i think the barcode you might have seen in the original report on Nanuq would map to the index (index is sometime called barcode by some people making this more confusing than it needs to be ;).
There was a problem hiding this comment.
you have the library on derived_sample__library__index__name and the sequences is available also at derived_sample__library__index__sequences_3prime and derived_sample__library__index__sequences_5prime
There was a problem hiding this comment.
Seeing the container barcodes there might actually be of some use if there is someone investigating the readset to check if there was some mixup ... It would be important though to mention that this is container_barcodes.
There was a problem hiding this comment.
Updated to keep the barcode information and expose it as container_barcodes .
|
|
||
|
|
||
| @action(detail=True, methods=["get"]) | ||
| def readsets(self, request, pk=None): |
There was a problem hiding this comment.
maybe call this overview_readsets to reduce the chance people might confuse what is sent back to normal readsets.
| ) | ||
| readset_file_paths = serializers.ListField(required=False) | ||
| readset_file_sizes = serializers.ListField(required=False) | ||
| validation_status = serializers.IntegerField(allow_null=True,) |
There was a problem hiding this comment.
Maybe call this run_validation_status instead to make it clear it is the flag that is assigned when validating the run. The library tab and sample will likely get a validation/qc_status flag also that will be differ from this one.
| const grouped = new Map<string, number>() | ||
|
|
||
| readsets.forEach((item) => { | ||
| const libraryType = item.library_type?.trim() || 'Non renseigné' |
There was a problem hiding this comment.
Might be better not to mix language in there. This is something that should essentially never happen (all sequenced sample should have a library_type... This may change but this would essentially be something wrong that would need investigating on our part.). For now you can use "unknown" or "sample without library".
| totalRuns: new Set(readsets.map((x) => x.run_name)).size, | ||
| totalSamples: new Set(readsets.map((x) => x.readset_sample_name)).size, | ||
| totalCohorts: new Set(readsets.map((x) => x.cohort)).size, | ||
| avgQuality: readsets.reduce((sum, x) => sum + Number(x.average_quality), 0) / total, |
There was a problem hiding this comment.
Averaging averages based on a variable number of bases is likely to skew the value... The correct way would be to use a weighted average using the number of bases for each readset . The number of bases is typically stored on the yield metrics field. I think some run do not provide this information you can assume an equal number of bases in this case.
There was a problem hiding this comment.
Fixed. I updated the average quality calculation to weight each readset by its yield (number of bases). The calculation falls back to an unweighted average when that information is incomplete.
| "alias": F("derived_sample__biosample__alias"), | ||
| "cohort": F("derived_sample__biosample__individual__cohort"), | ||
| "library_type": F("derived_sample__library__library_type__name"), | ||
| "number_of_reads": F("production_data__reads"), |
There was a problem hiding this comment.
This is not the best place to fetch the reads ... The production_data is extracted only once a run is validated and is not live... It is aggregated daily making this a bad source unless you want the have the reads that passed validation only... In our case, the source should be the readset metric nb_reads.
There was a problem hiding this comment.
Fixed. number_of_reads is now retrieved from the readset nb_reads metric instead of production_data.
| totalSamples: new Set(readsets.map((x) => x.readset_sample_name)).size, | ||
| totalCohorts: new Set(readsets.map((x) => x.cohort)).size, | ||
| avgQuality: readsets.reduce((sum, x) => sum + Number(x.average_quality), 0) / total, | ||
| avgAlignment: |
There was a problem hiding this comment.
For alignment and duplication you are averaging averages again ... ideally we would do a weighted average using the number of reads of the readset.
There was a problem hiding this comment.
Fixed. I updated both the alignment and duplication calculations to use weighted averages based on each readset’s number of reads.
|
|
||
| <Col xs={24} sm={12} lg={6} xl={3}> | ||
| <Card size="small" styles={{ body: { padding: '8px 12px' } }}> | ||
| <Statistic title="Cohortes" value={metrics.totalCohorts} /> |
| size="small" | ||
| bordered | ||
| scroll={{ x: 'max-content', y: 400 }} | ||
| pagination={{ |
There was a problem hiding this comment.
It would be great to have the number of hits listed in the pagination.
| onHeaderCell: compactHeaderCell, | ||
| filters: libraryTypeFilters, | ||
| filterIcon: (filtered) => <FilterOutlined style={{ color: filtered ? '#1677ff' : undefined }} />, | ||
| onFilter: (value, record) => record.library_type === value, |
| render: (name: string) => <Text strong>{name}</Text>, | ||
| }, | ||
| { | ||
| title: 'Sample', |
There was a problem hiding this comment.
Would be great to have a filter on that column too.
| <ProjectOverviewExportButton data={exportButtonData} /> | ||
| </div> | ||
| )} | ||
| {!isLoading && isActive && <ExternalIDReadSetDashboard readsets={projectOverviewReadsets} />} |
There was a problem hiding this comment.
I would like to test having the dashboard at the very top of the tab with the export button just above the table... This would allow for a clear filters button to be inserted also next to the export button.
| .includes(String(value).toLowerCase()), | ||
| }, | ||
| { | ||
| title: 'Run Start', |
There was a problem hiding this comment.
A filter here might be nice...
|
|
||
| const exportButtonData: ProjectOverviewExportButtonData = { | ||
| exportType: 'Project Readsets', | ||
| exportFunction: generateCsvContent, |
There was a problem hiding this comment.
Same comment as for the project... we typically send a request to the backend to export only the currently select items... Your csv builder is much more efficient than the package we are using ... I tested the big project MOH-YR5_CRCHUM ... The initial load time is quite large but the frontend filtering and export are much faster that way ... I am afraid that this might lead to some projects being undisplayable because the request times out... We will be adding more metrics to the query and this will slow down the call and some projects can run for years with potentially thousands of readsets ... Maybe it would be better to consider the overview_readsets to have its own endpoint to be able to use a similar behaviour as elsewhere. I am however curious if we could not improve our csv generation on the backend side.
There was a problem hiding this comment.
That makes sense. As discussed, could we address this in a follow-up ?
| <Row gutter={[16, 16]} style={{ margin: 8, borderRadius: 4 }}> | ||
| <Col xs={24} sm={12} lg={6} xl={3}> | ||
| <Card size="small" styles={{ body: { padding: '8px 12px' } }}> | ||
| <Statistic title="Readsets" value={metrics.totalReadsets} prefix={<ExperimentOutlined style={iconStyle('#1677ff', '#e6f4ff')}/>} /> |
There was a problem hiding this comment.
Numbers displayed should not use the , to separate thousands. You can use a space instead.
|
|
||
| <Col xs={24} sm={12} lg={6} xl={3}> | ||
| <Card size="small" styles={{ body: { padding: '8px 12px' } }}> | ||
| <Statistic title="Samples" value={metrics.totalSamples} prefix={<TeamOutlined style={iconStyle('#13c2c2', '#e6fffb')} />} /> |
There was a problem hiding this comment.
This will likely go to the thousands too so you should also replace the ,.
| "external_id": F("dataset__project__parent_project__external_id"), | ||
| "run_name": F("dataset__experiment_run__name"), | ||
| "lane": F("dataset__lane"), | ||
| "reference_genome_id": F("derived_sample__biosample__individual__reference_genome_id"), |
There was a problem hiding this comment.
This is not really informative without getting the assembly_name. This would need to be fetched separately from the store on the frontend or directly sent back (derived_sample__biosample__individual__reference_genome__assembly_name).
There was a problem hiding this comment.
Updated. The backend now returns the reference genome assembly name
…elated components
…er_barcodes and readset_files and sizes into json
…wsets, and frontend components
…DReadSetDashboard
|
When I exported on http://localhost:9000/external-projects-overview/193#readsets, the readset_files column shows up like this in the csv:
|
…s number of reads
…t button next to the table.
|
Closing this PR as it has been replaced by #1185, which consolidates the entire stack into a single PR. |







Note :
Please review PR #1180 first, if you haven't already.
This PR adds the Readsets tab. It follows the Project Submissions tab PR.