Claude/code review improvements my e wt#18
Open
Trophime wants to merge 5 commits intoseparate-coolingfrom
Open
Claude/code review improvements my e wt#18Trophime wants to merge 5 commits intoseparate-coolingfrom
Trophime wants to merge 5 commits intoseparate-coolingfrom
Conversation
Synthesizes two rounds of code review into a single, phased working plan covering readability, maintainability, extensibility, API integration (python_magnetapi), and interactive dashboards. https://claude.ai/code/session_01BFYugbXH42GbSWM9y1uSEA
One detailed, actionable prompt per improvement plan phase: - phase1_quick_wins.md — 10 targeted fixes (enum, logging, CI, paths) - phase2_architecture.md — module splits, config unification, tooling - phase3_extensibility.md — DataLoader protocol, registries, Parquet export - phase3b_api_integration.md — MagnetAPIClient, Pydantic models, upload/cache - phase4_dashboards.md — Panel/hvplot dashboards, notebook generator Each prompt includes: objective, prerequisites, per-task steps with code examples, and a verification checklist. https://claude.ai/code/session_01BFYugbXH42GbSWM9y1uSEA
Single reference document covering every library, tool, and stdlib module recommended across the five improvement phases: - Quick-reference table: package, version, pyproject group, mandatory phase - Packages to remove (iapws, nlopt, ht, requests shadow) - Target pyproject.toml shape with all extras - Per-phase breakdown with rationale notes - httpx vs requests rationale - Panel/hvplot vs Dash/Streamlit rationale - Minimum Python version justification (3.11) - Stdlib-only modules inventory https://claude.ai/code/session_01BFYugbXH42GbSWM9y1uSEA
The presence of FEPC hybrid data at 10 kHz (10 M+ points per run) requires dynamic, viewport-aware downsampling. plotly-resampler provides MinMaxLTTB aggregation server-side on every pan/zoom, which Panel/hvplot cannot match without bespoke pre-aggregation code. Changes: - software_stack.md: replace panel/hvplot/bokeh/holoviews rows with plotly/dash/plotly-resampler/tsdownsample in the quick-reference table and pyproject.toml shape; add FigureResampler vs FigureWidgetResampler table; add Panel vs Dash comparison table with large-time-series column; clarify requests→fetchers renaming must precede httpx adoption (ordering note now in both "Packages to remove" table and the Phase 3b rationale) - phase4_dashboards.md: rewrite context, prerequisites, Task 4.2 (widgets → figures.py with plain go.Figure builders), Task 4.3 (run_overview now uses FigureResampler + FigureWidgetResampler), Task 4.6 (hybrid_monitor fully rewritten around plotly-resampler — loads full-resolution kHz/RMS data, lets FigureResampler handle aggregation per viewport), Task 4.7 (__init__.py now exposes *_app/*_widget pairs), Task 4.8 (CLI drives Dash apps via app.run()), verification checklist updated, commit strategy updated https://claude.ai/code/session_01BFYugbXH42GbSWM9y1uSEA
Key findings from tool research: - Voilà (server): FigureWidgetResampler works — no design change needed - Voici (WASM): ipywidgets not fully supported, 2 GB browser memory cap → cannot do live resampling for FEPC data; pre-downsampled static figure only - Marimo: ipywidgets not supported (confirmed in issue #4091); use mo.ui.plotly() with plain go.Figure + tsdownsample LTTB Design changes: - Add a third output mode *_static(run, max_pts) to every dashboard: returns a plain go.Figure pre-downsampled via tsdownsample.LTTBDownsampler; works in Marimo, Voici, static HTML — no Dash or plotly-resampler needed - Split _require_dashboard_deps() into _require_plotly() (static path) and full guard (interactive path) so *_static() doesn't need Dash installed - Add hybrid_monitor_static() with t_start/t_end window narrowing for FEPC data: user selects time window first, then LTTB over that slice - Add marimo_generator.py: generates .py Marimo apps using mo.ui.plotly() + reactive slider for max_pts; includes overview and hybrid templates (with inline script metadata for uv/pipx dependency declaration) - Add to-marimo CLI subcommand alongside to-notebook - Add voila>=0.5 to [notebook] extras; add marimo>=0.9 as [marimo] extra - Document deployment compatibility matrix: *_app() → Dash; *_widget() → Jupyter+Voilà; *_static() → everywhere https://claude.ai/code/session_01BFYugbXH42GbSWM9y1uSEA
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.
No description provided.