refactor: control bar cleanup and filtering improvements#259
Draft
peymanvahidi wants to merge 20 commits into
Draft
refactor: control bar cleanup and filtering improvements#259peymanvahidi wants to merge 20 commits into
peymanvahidi wants to merge 20 commits into
Conversation
…lumbing Drops the Plane (XY/XZ/YZ) <select>, handlePlaneChange, the projection-plane-change CustomEvent, the projectionPlane @Property, and the cross-element assignment that pushed the plane onto the scatter-plot. Also drops projectionPlane? from ScatterplotElementLike. Helpers isProjection3D and getProjectionPlane are no longer imported here; they are deleted in a follow-up commit. The scatter-plot still owns its projectionPlane property in this commit; cleaned up next. Part of refactor for issue #196.
…gnment
Removes the projectionPlane Lit property, the xz/yz coordinate remap,
the point.z = coords[2] write (a write-only assignment with no readers),
and the projectionPlane entries in the _processData change-detection
gates. The data-processor call now passes four args; the unused fifth
parameter on data-processor itself is removed in a follow-up commit.
3D Parquet inputs now render using coords[0] / coords[1] only, matching
the historical default plane ('xy') that the property defaulted to in
production.
Part of refactor for issue #196.
Removes the projectionPlane parameter from DataProcessor.processVisualizationData and the conditional xz/yz coordinate remap. 3D projections now collapse to coords[0] / coords[1] on every code path. PlotDataPoint loses its optional z field; nothing consumed it (scatter-plot's only write was removed in the previous commit). Test changes: - Delete xz and yz plane-mapping tests. - Replace 'preserves z coordinate for 3D projections' with 'drops the z coordinate for 3D projections (rendered as 2D)', pinning the new contract. Part of refactor for issue #196.
…e helpers After the plane selector removal in earlier commits, these helpers have zero remaining callers in packages/ or app/. Removed both functions and their describe blocks from the test file. Part of refactor for issue #196.
…eric exclude path
…ders
The numeric filter's placeholder text showed the data's min/max
("min (4)", "max (1630)"). Removed at user request; placeholders are
now just "min" / "max". This also drops the now-unused
computeNumericBounds helper and the _dataBounds/_boundsAnnotation state
that only existed to feed those hints.
Owner
|
The filtering logic needs a complete revisit.
|
…257) The query builder evaluates against the full materialized dataset, but `_handleQueryApply` mapped the matched indices through `getCurrentData()` — the isolated subset after a prior apply — then stacked another `isolateSelection()` layer. Re-applying the same query resolved wrong/undefined ids and shrank the result each time (546 -> 19 -> only fading): the open #257 report. Route the query through the dedicated, idempotent `filteredProteinIds` / `filtersActive` channel the scatter plot already consumes, mapping indices through `getMaterializedData()` so they align with the array the query was evaluated against. A filter is no longer a selection or an isolation: re-applying is a no-op, manual isolation is left untouched, and the dead `isolate-data` / `reset-isolation` dispatches are dropped. Clear the filter on dataset swap (scatter-plot `updated()`, `applyPlotState`, and the control-bar data-change handler) so a stale filtered-id set can't blank the new plot or leave a stale Filter badge. Relabel "Apply & Isolate" -> "Apply Filter". Add control-bar.query-apply.test.ts pinning the idempotent-apply / reset / no-isolation contract.
…er NOT Three numeric-filter correctness fixes: - `_selectAnnotation` keyed on `kind === 'numeric'`, but the currently-coloured numeric annotation is materialized to `kind:'categorical'` (it keeps `sourceKind:'numeric'`), so picking it offered a categorical bin picker instead of the numeric range input. Use the shared `isNumericAnnotation()` so a numeric annotation is filtered numerically whether or not it is currently materialized. - An unconfigured numeric condition evaluated to the empty set, so NOT-wrapping it isolated EVERY protein (complement of nothing) while Apply stayed enabled — asymmetric with an empty categorical condition. Treat an unconfigured numeric condition as a no-op (matches all), mirroring categorical, so `NOT(unconfigured)` matches nothing and Apply stays disabled. - Switching operator left the now-hidden bound populated; switching back silently resurrected it and re-constrained the filter. Null the unused bound on operator change. Also give the operator select and min/max inputs accessible names (aria-label), and add component tests for the controlled round-trip, operator switch, numeric NOT-symmetry, and annotation kind detection.
- docs(explore): replace the removed XY/XZ/YZ plane-selector tip with a note that 3D projections render as their X/Y view (completes the #196 removal). - test(scatter-plot): drop z:0 from style-getters mock points; PlotDataPoint.z was removed in this PR. - refactor(query-types): stop seeding createGroup with a leading logicalOp:'AND' so a first-position group can't carry a spurious operator (mirrors createCondition; the builder sets the op for non-first groups). - test(control-bar): pin the flat left-to-right operator precedence ((A OR B) AND C) so the known limitation stays an intentional choice.
… follow-up) Routing query filters through the filteredProteinIds channel (df07c96) exposed a latent rendering bug. _processData built _plotData from _getCurrentDisplayData(), which COMPACTS the matched subset, so each PlotDataPoint.originalIndex became a slice-local 0..N index. But the style getters (_buildStyleGetters uses the full materialized data) and the tooltip path (this.data) resolve annotation values by originalIndex against the FULL dataset. For any non-prefix filter (e.g. protein_family=X, which keeps a scattered subset) every kept point was painted with the WRONG protein's colour/shape/opacity and showed the wrong tooltip — and points whose mis-resolved value was legend-hidden vanished. The channel was dormant before df07c96, so query-apply is what first made this reachable. Fix: build _plotData from the full materialized data and apply the matched-id set as a membership filter AFTER the map (a new optional `visibleProteinIds` arg on DataProcessor.processVisualizationData), exactly as isolation already does, so originalIndex stays a GLOBAL index. The style getters and tooltips — both written for global indices — now resolve correctly with no change. The fast path is gated off whenever a filter is active (it already rebuilt every time under the old slice-by-reference approach, so no perf change). getCurrentData()/data-change keep slicing for the legend and export, which want the filtered protein list. Add scatter-plot.filter-render.test.ts pinning per-point colour for a non-prefix filter, plus the prefix and unfiltered cases.
Collaborator
Author
|
@tsenoner I think the issues are fixed |
Brings in the multi-annotation hover tooltip, cross-projection duplicate badge grouping, and the includeShapes/shape-rotation removal from main. Conflict resolved in control-bar.ts: main inserted the `tooltipAnnotations` property immediately before the `projectionPlane` property this branch removed (3D plane selector teardown), so the two edits collided on adjacent lines. Kept main's `tooltipAnnotations`; dropped `projectionPlane`. The other five overlapping files (data-renderer.ts, control-bar/types.ts, scatter-plot.ts, style-getters.test.ts, utils/types.ts) auto-merged as disjoint hunks. Verified post-merge: type-check clean, 871 unit tests pass, and a per-file 3-way semantic audit confirmed each result equals the union of both branches' intent with no dropped or duplicated logic.
Owner
|
Hi @peymanvahidi, great progress.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Removes the 3D projection "Plane" (XY/XZ/YZ) dropdown and all
projectionPlaneplumbing from the web frontend. 3D Parquet files continue to load,zis silently discarded and onlyx/yare rendered. The PythonprotspaceCLI is unaffected.Changes:
handlePlaneChange,projection-plane-changeevent, theprojectionPlaneproperty, the cross-element assignment that pushed plane onto the scatter-plot, andprojectionPlane?fromScatterplotElementLike.projectionPlaneproperty, xz/yz coord remap,point.zwrite, andprojectionPlanefrom change-detection gates.DataProcessor.processVisualizationDatais now called with four args.projectionPlaneparameter and the xz/yz branch.PlotDataPointloses its optionalz?field. Tests updated: xz/yz mapping tests removed, "preserves z" test replaced with "drops the z coordinate for 3D projections."isProjection3DandgetProjectionPlanehelpers and their tests._updatePlotDataCoordinatesfast-path comment, it now correctly says "x/y" afterPlotDataPoint.zwas removed in commit 3.