Repository navigation
feat(workflow-operator): volcano plot significance cut-offs - #9041
eugenegujing wants to merge 5 commits into
Conversation
Add optional Significance threshold and Effect-size threshold properties that draw dashed cut-off lines at -log10(threshold) and at plus and minus the effect-size threshold. Add a Color by significance toggle that, with both thresholds set, colors points Up, Down or Not significant instead of by effect size. Bound both thresholds in the JSON schema and ignore a non-positive, non-finite or above-one value instead of drawing it.
Automated Reviewer SuggestionsBased on the
|
…ignificance-thresholds
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9041 +/- ##
============================================
- Coverage 91.85% 91.79% -0.06%
- Complexity 5020 5045 +25
============================================
Files 1279 1273 -6
Lines 56192 55610 -582
Branches 7320 7156 -164
============================================
- Hits 51615 51049 -566
- Misses 2741 2750 +9
+ Partials 1836 1811 -25
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
| config | throughput | MB/s | latency | max Δ latest / 7d | |
|---|---|---|---|---|---|
| 🔴 | bs=10 sw=10 sl=64 | 413 | 0.252 | 22,956/33,180/33,180 us | 🔴 +12.5% / 🔴 +157.4% |
| 🔴 | bs=100 sw=10 sl=64 | 840 | 0.513 | 115,420/171,508/171,508 us | 🔴 +25.5% / 🔴 +92.8% |
| ⚪ | bs=1000 sw=10 sl=64 | 986 | 0.602 | 1,009,955/1,055,749/1,055,749 us | ⚪ within ±5% / 🔴 +23.8% |
Baseline details
Latest main d6c9050 from same runner
| config | metric | PR | latest main | 7d avg | Δ latest | Δ 7d |
|---|---|---|---|---|---|---|
| bs=10 sw=10 sl=64 | throughput | 413 tuples/sec | 456 tuples/sec | 953.99 tuples/sec | -9.4% | -56.7% |
| bs=10 sw=10 sl=64 | MB/s | 0.252 MB/s | 0.278 MB/s | 0.582 MB/s | -9.4% | -56.7% |
| bs=10 sw=10 sl=64 | p50 | 22,956 us | 20,412 us | 10,584 us | +12.5% | +116.9% |
| bs=10 sw=10 sl=64 | p95 | 33,180 us | 32,778 us | 12,888 us | +1.2% | +157.4% |
| bs=10 sw=10 sl=64 | p99 | 33,180 us | 32,778 us | 16,266 us | +1.2% | +104.0% |
| bs=100 sw=10 sl=64 | throughput | 840 tuples/sec | 895 tuples/sec | 1,242 tuples/sec | -6.1% | -32.4% |
| bs=100 sw=10 sl=64 | MB/s | 0.513 MB/s | 0.546 MB/s | 0.758 MB/s | -6.0% | -32.3% |
| bs=100 sw=10 sl=64 | p50 | 115,420 us | 112,419 us | 83,370 us | +2.7% | +38.4% |
| bs=100 sw=10 sl=64 | p95 | 171,508 us | 136,617 us | 88,966 us | +25.5% | +92.8% |
| bs=100 sw=10 sl=64 | p99 | 171,508 us | 136,617 us | 101,186 us | +25.5% | +69.5% |
| bs=1000 sw=10 sl=64 | throughput | 986 tuples/sec | 988 tuples/sec | 1,276 tuples/sec | -0.2% | -22.8% |
| bs=1000 sw=10 sl=64 | MB/s | 0.602 MB/s | 0.603 MB/s | 0.779 MB/s | -0.2% | -22.7% |
| bs=1000 sw=10 sl=64 | p50 | 1,009,955 us | 1,015,961 us | 815,662 us | -0.6% | +23.8% |
| bs=1000 sw=10 sl=64 | p95 | 1,055,749 us | 1,046,858 us | 864,800 us | +0.8% | +22.1% |
| bs=1000 sw=10 sl=64 | p99 | 1,055,749 us | 1,046,858 us | 896,884 us | +0.8% | +17.7% |
Raw CSV
config_idx,batch_size,schema_width,string_len,num_batches,total_ms,total_tuples,total_bytes,tuples_per_sec,mb_per_sec,lat_p50_us,lat_p95_us,lat_p99_us
0,10,10,64,20,483.70,200,128000,413,0.252,22955.54,33179.69,33179.69
1,100,10,64,20,2381.79,2000,1280000,840,0.513,115420.42,171508.39,171508.39
2,1000,10,64,20,20278.61,20000,12800000,986,0.602,1009955.23,1055749.07,1055749.07
kz930
left a comment
There was a problem hiding this comment.
LGTM. The schema bounds plus the runtime filter cover both the property panel and hand-edited JSON.
…ignificance-thresholds # Conflicts: # common/workflow-operator/src/main/scala/org/apache/texera/amber/operator/visualization/volcanoPlot/VolcanoPlotOpDesc.scala
…ignificance-thresholds
| val sig = significanceCutoff.get | ||
| val eff = effectCutoff.get | ||
| pyb"""significant = table[$pvalueColumn] < $sig | ||
| | table["Significance"] = np.select( |
There was a problem hiding this comment.
Writing the categories into table["Significance"] overwrites an input column of that name before px reads the table. If that column is the selected effect column, the x axis turns into the Up / Down / Not significant labels (I checked with plotly 5.24.1). Passing the categories straight to px leaves the table alone: significance = np.select(...), then color=significance, labels={"color": "Significance"} and category_orders={"color": [...]} keep the same legend title, colors and order. The standalone twin below needs the same change.
| it should "draw a dashed horizontal line at -log10 of the significance threshold" in { | ||
| val op = configured | ||
| op.significanceThreshold = Some(0.05) | ||
| val code = op.generatePythonCode() | ||
| code should include(hline) | ||
| code should not include "add_vline" | ||
| assertContinuousColoring(code) | ||
| } | ||
|
|
||
| it should "draw dashed vertical lines at plus and minus the effect-size threshold" in { | ||
| val op = configured | ||
| op.effectSizeThreshold = Some(1.0) | ||
| val code = op.generatePythonCode() | ||
| code should include(vlines) | ||
| code should not include "add_hline" | ||
| assertContinuousColoring(code) | ||
| } |
There was a problem hiding this comment.
A few of the new tests pin nothing another one doesn't:
- These two single-threshold tests: the fallback test further down already checks that each threshold alone draws only its own lines with the continuous coloring, and the toggle-off test checks the lines don't depend on the toggle.
- "hold Doubles" repeats "deserialize numeric strings", which already fails without
contentAs. - The export path's toggle-off and saved-workflow tests: both paths draw the lines from the same
thresholdLines, the parity test covers the toggle-off row, and the engine's saved-workflow test already shows a saved operator matches a default one (its assertions after the equality check can go too).
What changes were proposed in this PR?
The Volcano Plot draws a plain scatter of effect size vs
-log10(p), colored by effect size, which is already the x axis. Nothing shows where the significance bar is or which points clear it. This PR adds three optional properties, all off by default, so saved workflows render as before.0 < x <= 1-log10(threshold)> 0+thresholdand-thresholdUp/Down/Not significantwith a legend instead of the continuousRdBuscaleCategories use strict comparisons on the selected p-value column:
Up=p < sigandeffect > eff,Down=p < sigandeffect < -eff, everything elseNot significant. The category is written to a column namedSignificance(also the legend title); an input column of that name would be shadowed inside the operator only. Colors are ColorBrewer RdBu end points#B2182B/#2166ACplus grey#9E9E9E.Validation: the JSON schema bounds above stop bad values in the property panel. For hand-edited JSON the operator also ignores a threshold it cannot draw (NaN, infinite, <= 0, or a p cut-off > 1) and falls back to the continuous coloring if the toggle is on but a threshold is missing. A cut-off of exactly 1 is allowed and draws its line at y = 0. If the cut-off's
-log10is beyond the data range the line may fall outside the auto-ranged axis; extending the range is left for a follow-up.Since #8504 merged, the operator also exports a standalone script through
generateStandaloneCode(). This PR adds the cut-off lines, theSignificancecolumn and the three-color map there too, from the same helpers where possible: the color map and category order are one shared literal, the threshold lines come from the same builder, and only the column names differ (Python literals instead of decoded strings). A spec checks that both paths make the same line and coloring decisions for eight threshold shapes, so the exported figure cannot drift from the engine's.Independent of the label-fix (#9037) and hover-column (#9038) PRs; all three touch neighbouring lines of
VolcanoPlotOpDesc.scala, so whichever lands second needs a small rebase.Figures below are the operator's generated Python from
mainand from this branch, run on the 430-gene example workflow from the issue, with thresholds 0.05 and 1.Any related issues, documentation, discussions?
Closes #9039
PR #8504 (merged) added the
generateStandaloneCode()export path; the mirroring described above is the follow-up it needed.How was this PR tested?
New tests in
VolcanoPlotOpDescSpec, written before the source change:np.selectcolumn, exact color map and category order, nocolor_continuous_scaleInfinity,-Infinity,NaNfrom JSON never reach the Python1e-300,Double.MIN_VALUEand1e21in every site they appearexclusiveMinimum/maximumon the two thresholdsgenerateStandaloneCode: defaults draw no line and keep the continuous coloring; thresholds with the toggle off draw the dashed lines only; the toggle on colorsUp/Down/Not significantwith the column names as literals; a cross-path test checks that the exported script makes the same cut-off and coloring decisions as the engine for eight threshold shapes (unset, one side only, zero, p cut-off above 1, non-finite, exactly 1); a saved workflow renders as a default-constructed operatorSmoke run outside Texera (pytexera stubbed, pandas + plotly 5.24.1) on the 430-gene mock table, 420 rows after the operator drops zero and missing p-values:
Up35,Down35,Not significant350"Infinity"/"NaN"from JSONNot significant420Up41,Down38,Not significant341The exponent-form dumps pass
python -m py_compile. The exported script for the default, lines-only and colored configurations parses withast.parse.Was this PR authored or co-authored using generative AI tooling?
Yes. Generated-by: Claude Code (claude-fable-5-1). Co-authored with Claude; the author reviewed the change before submission.