AA abundance per position plot - #37
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a new 'AA abundance per position' plot, adding the required UI state, data outputs, and routing. It also includes a comprehensive update of project dependencies. The review feedback identifies an opportunity to improve the model's efficiency and maintainability by refactoring redundant data retrieval logic into a single internal output.
| .outputWithStatus('aaPositionPf', (ctx) => { | ||
| const datasetRef = ctx.args.datasetRef; | ||
| if (datasetRef === undefined) return undefined; | ||
|
|
||
| const pCols = ctx.resultPool.getAnchoredPColumns( | ||
| { main: datasetRef }, | ||
| [{ | ||
| axes: [ | ||
| { anchor: 'main', idx: 0 }, | ||
| { anchor: 'main', idx: 1 }, | ||
| { name: 'pl7.app/vdj/chain' }, | ||
| { name: 'pl7.app/vdj/numberingPosition' }, | ||
| ], | ||
| }], | ||
| ); | ||
|
|
||
| if (!pCols || pCols.length === 0) return undefined; | ||
| return ctx.createPFrame(pCols); | ||
| }) | ||
|
|
||
| .output('aaPositionPCols', (ctx) => { | ||
| const datasetRef = ctx.args.datasetRef; | ||
| if (datasetRef === undefined) return undefined; | ||
|
|
||
| const pCols = ctx.resultPool.getAnchoredPColumns( | ||
| { main: datasetRef }, | ||
| [{ | ||
| axes: [ | ||
| { anchor: 'main', idx: 0 }, | ||
| { anchor: 'main', idx: 1 }, | ||
| { name: 'pl7.app/vdj/chain' }, | ||
| { name: 'pl7.app/vdj/numberingPosition' }, | ||
| ], | ||
| }], | ||
| ); | ||
|
|
||
| if (!pCols || pCols.length === 0) return undefined; | ||
| return pCols.map((c) => ({ columnId: c.id, spec: c.spec } satisfies PColumnIdAndSpec)); | ||
| }) | ||
|
|
||
| .output('isRunning', (ctx) => ctx.outputs?.getIsReadyOrError() === false) | ||
|
|
||
| .title(() => 'CDR3 Spectratype') | ||
|
|
||
| .subtitle((ctx) => ctx.args.customBlockLabel || ctx.args.defaultBlockLabel) | ||
|
|
||
| .sections((_) => [ | ||
| { type: 'link', href: '/', label: 'Bubble Plot' }, | ||
| { type: 'link', href: '/vStackedBarPlot', label: 'V Spectratype' }, | ||
| { type: 'link', href: '/cdr3StackedBarPlot', label: 'CDR3 Spectratype' }, | ||
| ]) | ||
| .sections((ctx) => { | ||
| const datasetRef = ctx.args?.datasetRef; | ||
| const hasAaPositions = datasetRef | ||
| ? (ctx.resultPool.getAnchoredPColumns( | ||
| { main: datasetRef }, | ||
| [{ | ||
| axes: [ | ||
| { anchor: 'main', idx: 0 }, | ||
| { anchor: 'main', idx: 1 }, | ||
| { name: 'pl7.app/vdj/chain' }, | ||
| { name: 'pl7.app/vdj/numberingPosition' }, | ||
| ], | ||
| }], | ||
| ) ?? []).length > 0 | ||
| : false; | ||
|
|
||
| return [ | ||
| { type: 'link', href: '/', label: 'Bubble Plot' }, | ||
| { type: 'link', href: '/vStackedBarPlot', label: 'V Spectratype' }, | ||
| { type: 'link', href: '/cdr3StackedBarPlot', label: 'CDR3 Spectratype' }, | ||
| ...(hasAaPositions ? [{ type: 'link' as const, href: '/aaPositionPlot' as const, label: 'AA Position' }] : []), | ||
| ]; | ||
| }) |
There was a problem hiding this comment.
The call to ctx.resultPool.getAnchoredPColumns with identical arguments is repeated three times in outputWithStatus('aaPositionPf', ...), output('aaPositionPCols', ...), and sections(...). This is inefficient, especially if getAnchoredPColumns is an expensive operation, and it also leads to code duplication, which can make maintenance more difficult.
A better approach would be to create a single intermediate output to compute the columns once. The result can then be reused in the other outputs and sections, which will improve performance and maintainability.
.output('internalAaPositionPCols', (ctx) => {
const datasetRef = ctx.args.datasetRef;
if (datasetRef === undefined) return undefined;
const pCols = ctx.resultPool.getAnchoredPColumns(
{ main: datasetRef },
[{
axes: [
{ anchor: 'main', idx: 0 },
{ anchor: 'main', idx: 1 },
{ name: 'pl7.app/vdj/chain' },
{ name: 'pl7.app/vdj/numberingPosition' },
],
}],
);
return (pCols && pCols.length > 0) ? pCols : undefined;
})
.outputWithStatus('aaPositionPf', (ctx) => {
const pCols = ctx.outputs.internalAaPositionPCols;
if (pCols === undefined) return undefined;
return ctx.createPFrame(pCols);
})
.output('aaPositionPCols', (ctx) => {
const pCols = ctx.outputs.internalAaPositionPCols;
if (pCols === undefined) return undefined;
return pCols.map((c) => ({ columnId: c.id, spec: c.spec } satisfies PColumnIdAndSpec));
})
.output('isRunning', (ctx) => ctx.outputs?.getIsReadyOrError() === false)
.title(() => 'CDR3 Spectratype')
.subtitle((ctx) => ctx.args.customBlockLabel || ctx.args.defaultBlockLabel)
.sections((ctx) => {
const hasAaPositions = ctx.outputs.internalAaPositionPCols !== undefined;
return [
{ type: 'link', href: '/', label: 'Bubble Plot' },
{ type: 'link', href: '/vStackedBarPlot', label: 'V Spectratype' },
{ type: 'link', href: '/cdr3StackedBarPlot', label: 'CDR3 Spectratype' },
...(hasAaPositions ? [{ type: 'link' as const, href: '/aaPositionPlot' as const, label: 'AA Position' }] : []),
];
})
This PR adds AA abundance per position plot to cdr3-spectratype block. This PR depends on platforma-open/redefine-clonotypes#25.