Improve implicit fitting backend and workspace flows#47
Conversation
📝 Walkthrough</validation_retry> ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
docs/desktop/fitting.en.md (1)
3-17:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon't describe the page as explicit-only if it also documents implicit fitting.
The intro and section lead-in both say "explicit" fitting, but the bullet list now includes self-consistent/implicit models. Reword this page to say the desktop fitting module supports both explicit and self-consistent/implicit models.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/desktop/fitting.en.md` around lines 3 - 17, Update the wording in the intro and the "Model Selection" lead-in so they no longer claim the module is "explicit-only": replace occurrences of the phrase "explicit models" and "explicit fitting models" with language that includes both explicit and self-consistent/implicit models (e.g., "explicit and self-consistent/implicit models" or "explicit and implicit models") so the description matches the listed bullet that includes "self-consistent/implicit models"; ensure the first sentence ("The fitting module fits explicit models to your data.") and the "Model Selection" heading text are edited accordingly.app_desktop/workspace_controller.py (1)
797-802:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore the explicit constraints toggle before repopulating parameter rows.
_restore_param_rows()flows throughParameterTable.set_rows(), which normalizes with the table's currentconstraints_enabledflag. Here that happens beforecustom_constraints_checkboxis restored, so a workspace saved with explicitfixed/min/maxvalues will reopen with those fields stripped if the table is still in its default unconstrained state.Suggested fix
- parameter_rows = fitting.get("parameter_rows") or _legacy_parameter_text_rows(fitting.get("parameters")) - _restore_param_rows(window, parameter_rows, fitting.get("parameter_orphans")) - if not parameter_rows and isinstance(fitting.get("parameters"), list): - _restore_param_rows(window, fitting.get("parameters")) if hasattr(window, "custom_constraints_checkbox"): window.custom_constraints_checkbox.setChecked(bool(fitting.get("constraints_enabled"))) + parameter_rows = fitting.get("parameter_rows") or _legacy_parameter_text_rows(fitting.get("parameters")) + _restore_param_rows(window, parameter_rows, fitting.get("parameter_orphans")) + if not parameter_rows and isinstance(fitting.get("parameters"), list): + _restore_param_rows(window, fitting.get("parameters"))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app_desktop/workspace_controller.py` around lines 797 - 802, Restore the explicit constraints toggle (window.custom_constraints_checkbox / constraints_enabled) before repopulating parameter rows so ParameterTable.set_rows sees the correct constraints state; move the block that sets window.custom_constraints_checkbox.setChecked(bool(fitting.get("constraints_enabled"))) to occur before calls to _restore_param_rows(parameter_rows, ...) (and the fallback _restore_param_rows for fitting.get("parameters")), or ensure you set the table's constraints_enabled flag directly before calling _restore_param_rows, so ParameterTable.set_rows will not normalize away fixed/min/max fields.docs/desktop/fitting.zh.md (1)
3-18:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win避免把该页面写成“仅显式拟合”。
这里前文写的是“显式模型拟合”，后面却把“自洽隐式模型”列进支持范围，表述前后矛盾。建议改成“支持显式模型与自洽/隐式模型拟合”。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/desktop/fitting.zh.md` around lines 3 - 18, The intro currently says "显式模型拟合" but later lists "自洽隐式模型", causing a contradiction; update the heading/first sentence (the phrase "拟合模块用于对数据进行显式模型拟合") to state that the module supports both explicit and self-consistent/implicit models (e.g., change to "拟合模块用于对数据进行显式与自洽/隐式模型拟合"), and ensure the terms "显式模型拟合" and "自洽隐式模型" used elsewhere in the document are consistent with that wording.app_web/logic/fitting.py (1)
636-648:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject weighted fits when uncertainties are missing or partial.
When
fit_weightedis enabled, this branch still falls back tosigmas_for_fit = Noneif no uncertainty series is present, and it can also forward a partially populated list containingNone. That means the UI can say “weighted” while the backend actually runs unweighted or depends on downstream handling of missing sigmas.Suggested guard
sigma_list: list[mp.mpf | None] | None = None if sigma_column: sigma_list = _column_series(headers, rows, sigma_column) else: target_idx = headers.index(target_column) collected: list[mp.mpf | None] = [] for sig_row in sigma_rows: entry = sig_row[target_idx] if target_idx < len(sig_row) else None collected.append(mp.mpf(entry) if entry is not None else None) if any(val is not None for val in collected): sigma_list = collected + if use_weights: + if not sigma_list: + raise ValueError( + _dual_msg( + "加权拟合要求提供不确定度列或带不确定度的目标数据。", + "Weighted fitting requires a sigma column or uncertainties on the target data.", + ) + ) + if any(val is None for val in sigma_list): + raise ValueError( + _dual_msg( + "加权拟合要求每个数据点都有不确定度。", + "Weighted fitting requires an uncertainty for every data point.", + ) + ) sigmas_for_fit = sigma_list if (use_weights and sigma_list) else None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app_web/logic/fitting.py` around lines 636 - 648, The code currently allows use_weights to be true while sigmas_for_fit becomes None or contains None values; update the post-processing logic so that if use_weights (aka fit_weighted) is requested you either accept a fully-populated sigma list or explicitly reject the weighted fit: after building sigma_list (and using sigma_rows/target_idx logic), validate that sigma_list is not None, has the same length as rows (or the expected data series) and contains no None entries; only then set sigmas_for_fit = sigma_list, otherwise raise a clear exception or return an error indicating missing/partial uncertainties (and set sigmas_for_fit = None as fallback) so the backend never silently runs an "unweighted" fit when use_weights is requested.datalab_latex/derivatives.py (2)
218-228:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSame symbol source mismatch in
_build_symbolic_partials.Same issue as in
_build_symbolic_hessian- thesymbolslist from line 218 may not match the symbols in the parsedexpr.🐛 Proposed fix
- symbols, _ = _build_sympy_local_dict(variables) - try: - expr, _ = parse_symbolic_expression( + expr, symbol_map = parse_symbolic_expression( normalized, variables=variables, normalize=False, evaluate=True, ) except Exception: return None + symbols = [symbol_map[name] for name in variables]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@datalab_latex/derivatives.py` around lines 218 - 228, In _build_symbolic_partials, the precomputed symbols from _build_sympy_local_dict(variables) can differ from the actual symbols parsed into expr by parse_symbolic_expression; after successfully parsing expr (in the try block where parse_symbolic_expression is called), regenerate or reconcile the symbols used for differentiation by deriving the symbol list from the parsed expr (e.g., use expr.free_symbols or call _build_sympy_local_dict with the symbols present in expr) and use that authoritative symbol mapping for subsequent operations instead of the earlier `symbols` variable; ensure this happens before any differentiation or substitution and preserve the existing exception handling that returns None on parse failure.
144-154:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSymbol source mismatch between
build_sympy_local_dictandparse_symbolic_expression.
_build_sympy_local_dictreturns one set of Symbol objects, whileparse_symbolic_expressioninternally callsbuild_sympy_local_dictagain and returns a different symbol map. Line 144 extractssymbolsfrom one call, but theexprfrom line 147 uses symbols from a separate internal call. When these symbols are used insp.diff(expr, symbols[i], ...)at line 171, they may not match the symbols actually present inexpr.Consider using the symbol map returned by
parse_symbolic_expressioninstead:🐛 Proposed fix
- symbols, _ = _build_sympy_local_dict(variables) - try: - expr, _ = parse_symbolic_expression( + expr, symbol_map = parse_symbolic_expression( normalized, variables=variables, normalize=False, evaluate=True, ) except Exception: return None + symbols = [symbol_map[name] for name in variables]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@datalab_latex/derivatives.py` around lines 144 - 154, The code calls _build_sympy_local_dict to get symbols but then calls parse_symbolic_expression which internally rebuilds symbols and returns its own symbol map (second return value), causing a mismatch; fix by removing or ignoring the initial _build_sympy_local_dict call and instead capture and use the symbol map returned by parse_symbolic_expression (i.e., change "expr, _ = parse_symbolic_expression(...)" to "expr, symbols = parse_symbolic_expression(...)" and use that symbols in subsequent sp.diff calls such as where symbols[i] is referenced), ensuring normalize/evaluate flags remain as intended and handling the exception path unchanged.
🟡 Minor comments (12)
docs/DATALAB_WEB_GUIDE.en.md-37-41 (1)
37-41:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocument self-consistent fitting in the main web guide too.
These updated lists omit the self-consistent fitting mode even though this PR’s transition is explicit/custom/self-consistent fitting. That leaves the primary web guide stale for one of the new user-facing workflows.
Also applies to: 165-169
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/DATALAB_WEB_GUIDE.en.md` around lines 37 - 41, The web guide lists of fitting modes currently omit "Self-consistent fitting"; update both occurrences of the lists containing "Polynomial fitting", "Inverse-power series fitting", "Padé approximation", "Power-limit template", "Custom models" to include "Self-consistent fitting" (e.g., insert "Self-consistent fitting" into those bullet lists so the main guide matches the new transition that introduces explicit/custom/self-consistent modes).docs/ARCHITECTURE.md-74-74 (1)
74-74:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPoint readers at
workers_qt.pyfor the actual worker implementations.
CalcJobandFitJobare the payloads fromworkers_core.py; the worker wrappers themselves live inworkers_qt.py. As written, this sends contributors to the wrong module when they need thread lifecycle or shutdown behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/ARCHITECTURE.md` at line 74, Update the ARCHITECTURE.md description so readers are directed to workers_qt.py for the actual worker implementations and thread/shutdown behavior: state that CalcJob and FitJob are payload/data classes defined in workers_core.py while the worker wrappers (e.g., FitWorker, CalcWorker) and lifecycle/shutdown logic live in workers_qt.py, and replace the current line that points contributors to workers_core.py with this clarification.docs/DATALAB_WEB_GUIDE.md-37-41 (1)
37-41:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win把自洽/隐式拟合也写进这里。
这两处更新后的列表都漏掉了本次改造里的自洽拟合模式，导致 Web 主指南描述的能力比实际 UI 少一项，用户很难从文档里发现这条工作流。
Also applies to: 165-169
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/DATALAB_WEB_GUIDE.md` around lines 37 - 41, 在两个更新后的模型列表中缺少“自洽/隐式拟合”条目（出现在当前 diff 的多项式拟合、反幂级数拟合、Padé 近似、幂律极限模板、自定义模型 列表），请在这两个位置（原注释提到的两处列表）将“自洽/隐式拟合”以与其它项一致的条目格式插入到合适位置（例如多项式拟合之后或与其他拟合模型并列），确保文档中 Web 主指南的模型列表与 UI 中实际可选项保持一致并在两处列表（另一个在文档后半段的相应列表）都做相同修改。app_desktop/formula_preview.py-163-168 (1)
163-168:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon’t advertise non-interactive labels as clickable.
configure_formula_preview_label()always sets the hand cursor and “Click to enlarge formula”, but Line 194 only attaches preview state whenset_preview_sourceexists. A plainQLabelcan now look interactive and then do nothing on click.Suggested fix
def configure_formula_preview_label(label: QLabel) -> None: label.setWordWrap(True) label.setMaximumWidth(_INLINE_PREVIEW_MAX_WIDTH + 20) label.setSizePolicy(QSizePolicy.Policy.Ignored, QSizePolicy.Policy.Fixed) - label.setCursor(Qt.CursorShape.PointingHandCursor) - label.setToolTip("Click to enlarge formula") + if hasattr(label, "set_preview_source"): + label.setCursor(Qt.CursorShape.PointingHandCursor) + label.setToolTip("Click to enlarge formula") + else: + label.unsetCursor() + label.setToolTip("")Also applies to: 194-195
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app_desktop/formula_preview.py` around lines 163 - 168, The label is always styled as clickable in configure_formula_preview_label (hand cursor and "Click to enlarge formula") even when no preview handler is attached; change configure_formula_preview_label to only set the pointing-hand cursor and the "Click to enlarge formula" tooltip when the given QLabel has the preview behavior attached (i.e., when it has a callable attribute/method set_preview_source or when the caller will attach a preview), otherwise leave the default cursor/tooltip; also update the code path that attaches preview (the code that calls set_preview_source) to ensure it explicitly sets the cursor and tooltip there when attaching the preview handler so the label appearance matches its interactivity.app_desktop/fitting_latex_writer.py-237-239 (1)
237-239:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReport failed SciPy safety checks as failed, not “not used”.
When
scipy_safety_passedis present and false, this renders"not used". That loses the distinction between “SciPy was skipped” and “SciPy was evaluated and rejected”, which is exactly the backend evidence this PR is trying to surface.Proposed fix
- if "scipy_safety_passed" in fit_result.details: - status = "passed" if bool(fit_result.details.get("scipy_safety_passed")) else "not used" + if "scipy_safety_passed" in fit_result.details: + status = "passed" if bool(fit_result.details.get("scipy_safety_passed")) else "failed" solver_details.append(f"SciPy precision check: {status}")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app_desktop/fitting_latex_writer.py` around lines 237 - 239, The current block maps fit_result.details["scipy_safety_passed"] False to "not used" which hides that SciPy was evaluated and failed; in the block that checks for "scipy_safety_passed" update the status mapping so that when bool(fit_result.details.get("scipy_safety_passed")) is True it sets "passed" and when False it sets "failed" (leave the surrounding logic that appends to solver_details intact), referencing the fit_result.details key "scipy_safety_passed" and the solver_details.append call so the message becomes "SciPy precision check: passed" or "SciPy precision check: failed".tests/test_auto_fit_removed.py-165-179 (1)
165-179:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNarrow the doc blacklist so removal notes can still be documented.
This flags raw substrings like
"auto-fit"anywhere, so a changelog entry such as “auto-fit was removed” would fail even though it is exactly the documentation we want for this breaking change. Please scope this to positive feature claims or exclude removal/deprecation contexts.Also applies to: 213-228
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_auto_fit_removed.py` around lines 165 - 179, The current banned_claims tuple matches raw substrings (e.g., "auto-fit") and will false-positive on changelog text like "auto-fit was removed"; change banned_claims to a list of regex patterns (or tuple of regex strings) that use word boundaries (e.g., r"\bauto-fit\b") and update the matching logic to only flag when a positive-claim context is present by skipping matches if the surrounding text contains removal/deprecation cues (e.g., "remove", "removed", "removal", "deprecated", "deprecate") within a short window; reference the banned_claims variable and the function/method that performs the blacklist check so you can swap substring checks for regex search + a simple context check to exempt removal/deprecation notes.tests/test_app_desktop_workers_core.py-587-599 (1)
587-599:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon't couple this test helper to a
.gitcheckout.
git ls-filesmakes the AST guard environment-dependent; source archives and some CI sandboxes can fail here before the test inspects any Python code. Walking the declared paths directly keeps the test hermetic.♻️ Suggested change
def _tracked_production_python_files(root: Path) -> list[Path]: - result = subprocess.run( - ["git", "ls-files", "--", *_PRODUCTION_PYTHON_PATHS], - cwd=root, - check=True, - capture_output=True, - text=True, - ) - return [ - root / path - for path in result.stdout.splitlines() - if path.endswith(".py") - ] + files: list[Path] = [] + for relpath in _PRODUCTION_PYTHON_PATHS: + path = root / relpath + if path.is_file() and path.suffix == ".py": + files.append(path) + elif path.is_dir(): + files.extend(sorted(path.rglob("*.py"))) + return files🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_app_desktop_workers_core.py` around lines 587 - 599, The helper _tracked_production_python_files currently shells out to "git ls-files", coupling tests to a git checkout; replace that logic with a hermetic filesystem walk over each path in _PRODUCTION_PYTHON_PATHS (using Path.rglob or os.walk) rooted at root, collecting files that end with ".py" and returning them as root / relative_path Paths so the function no longer depends on a .git repository and works in source archives and CI sandboxes.docs/web/fitting.zh.md-3-3 (1)
3-3:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThe intro narrows fitting support too much.
This now says the feature only does
显式模型fitting, but this PR also adds self-consistent/implicit fitting flows and examples. Please mention both explicit and self-consistent/implicit modes here so the page matches the feature set.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/web/fitting.zh.md` at line 3, The intro sentence currently restricts fitting to "显式模型" only; update the opening line that reads "对数据进行显式模型曲线拟合，获取拟合参数并比较模型指标。" to mention both explicit (显式模型) and self-consistent/implicit (自洽/隐式) fitting flows and examples so the page covers both modes and matches the PR's added features.app_desktop/window_fitting_mixin.py-64-71 (1)
64-71:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMirror the fallback status message for batch fits too.
This only runs for
FitWorkercompletions.FitBatchWorkerfinishes through_on_fit_batches_finished, so the new fallback diagnostic disappears as soon as the user fits more than one batch. Scanning the batch entries for anyfit_result.details["fallback_history"]would keep the behavior consistent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app_desktop/window_fitting_mixin.py` around lines 64 - 71, The fallback message currently only appears in _on_fit_finished for single FitWorker runs; add the same diagnostic to _on_fit_batches_finished by iterating the batch results (the payload passed to _on_fit_batches_finished), inspecting each entry's fit_result.details (safely via getattr and defaulting to {}), and if any details.get("fallback_history") is truthy call self.statusBar().showMessage with the same translated string and timeout; reference _on_fit_batches_finished, fit_result.details, and "fallback_history" to locate where to add this check so batch fits mirror the single-fit behavior.docs/desktop/index.en.md-11-11 (1)
11-11:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInclude implicit/self-consistent fitting in the module summary.
The desktop fitting surface in this PR is no longer explicit-only. Describing the module as just “fit explicit models” makes the index page contradict the shipped self-consistent / implicit workflow.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/desktop/index.en.md` at line 11, Update the summary line that currently reads "Fitting: fit explicit models and output parameters, metrics, residuals and curves" to reflect that the desktop fitting surface supports implicit/self-consistent workflows as well; edit the string (the "Fitting:" summary) to something like "Fitting: fit explicit and implicit (self‑consistent) models and output parameters, metrics, residuals and curves" so the module description no longer implies explicit-only behavior.docs/desktop/index.zh.md-11-11 (1)
11-11:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win补上隐式/自洽拟合能力的描述。
这里写成“显式模型拟合”后，首页概览就和当前已经提供的自洽/隐式拟合能力不一致了。建议在模块简介里一并点出这条能力，避免用户误判功能范围。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/desktop/index.zh.md` at line 11, Replace the current single-line description "拟合：对数据进行显式模型拟合，输出参数、不确定度与拟合优度信息" with wording that also mentions implicit/self-consistent fitting (e.g. "显式/隐式（自洽）拟合：对数据进行显式模型拟合或隐式/自洽拟合，输出参数、不确定度与拟合优度信息"), or append a short sentence after that line clarifying the module supports both explicit model fitting and implicit/self-consistent fitting; update the phrase in index.zh.md where the exact string "拟合：对数据进行显式模型拟合，输出参数、不确定度与拟合优度信息" appears to ensure users won’t misinterpret the feature scope.app_web/blueprints/sse.py-275-308 (1)
275-308:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReject non-streamable models during request validation.
_resolve_model_id()currently acceptscustom,pade, andpower_limit, but_single_fit_events()only supportspolynomialandinverse_power. That means/api/fit/stream?model=customgets past validation, emitsstarted, and only then fails asUnsupportedModel, while the validation error text also claims those models are supported. Tightening_PUBLIC_SSE_MODEL_ALIASESto the streamable set would keep the SSE contract honest and fail fast.Also applies to: 371-380
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app_web/blueprints/sse.py` around lines 275 - 308, The public alias map currently advertises non-streamable models; restrict _PUBLIC_SSE_MODEL_ALIASES to only streamable models (keep "polynomial"/"poly" and "inverse_power"/"inverse" entries, remove "pade", "power_limit", "custom"), so _resolve_model_id only accepts streamable models; update the unsupported-model error text in _resolve_model_id to list the actual supported streamable models ("polynomial", "inverse_power") and remove mention of the removed ones; also check the duplicate copy of this logic referenced around the later block (the other occurrence at the 371-380 area) and make the same alias and error-message changes so validation fails fast for non-streamable models.
🧹 Nitpick comments (8)
app_desktop/formula_preview.py (1)
51-70: ⚡ Quick winReuse
FormulaPreviewDialogfor label clicks.This opens a second, simpler dialog path and skips the shared Copy action and fallback behavior already implemented in
FormulaPreviewDialog. Calling the shared helper here keeps both entry points in sync.Suggested fix
def mousePressEvent(self, event) -> None: # type: ignore[no-untyped-def] if event.button() != Qt.MouseButton.LeftButton or not self._preview_expression.strip(): super().mousePressEvent(event) return - dialog = QDialog(self) - dialog.setWindowTitle("Formula") - layout = QVBoxLayout(dialog) - expanded = QLabel() - expanded.setAlignment(Qt.AlignmentFlag.AlignCenter) - pixmap = render_formula_pixmap(self._preview_expression, lhs=self._preview_lhs) - if pixmap is not None and not pixmap.isNull(): - expanded.setPixmap(pixmap) - else: - expanded.setText(self._preview_expression) - layout.addWidget(expanded) - dialog.resize( - max(420, expanded.sizeHint().width() + 48), - max(180, expanded.sizeHint().height() + 48), - ) - dialog.exec() + open_formula_preview_dialog( + self, + self._preview_expression, + lhs=self._preview_lhs, + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app_desktop/formula_preview.py` around lines 51 - 70, The mousePressEvent currently builds a bespoke QDialog and duplicates logic that exists in FormulaPreviewDialog; replace the custom dialog creation in mousePressEvent with creating and showing FormulaPreviewDialog (pass self as parent and forward self._preview_expression and self._preview_lhs), so the shared Copy action, fallback text behavior, sizing and any other shared logic implemented in FormulaPreviewDialog are used instead of duplicating them; call the dialog's exec() (or exec_() if that class uses it) to display it.tests/test_implicit_packaging.py (1)
13-21: ⚡ Quick winParse
DataLab.specstructurally instead of matching formatting.These regexes will fail on harmless refactors like comments, line wrapping, or tuple/list reformatting even when packaging still includes the right modules. AST-based assertions would track the Python values instead of the current source layout.
♻️ Suggested direction
+import ast import re def test_pyinstaller_packaging_collects_sympy() -> None: root = Path(__file__).resolve().parents[1] spec = (root / "DataLab.spec").read_text(encoding="utf-8") @@ - hidden_imports = re.search(r"hiddenimports\s*=\s*\[(?P<body>.*?)\]", spec, re.S) - assert hidden_imports is not None - for package in ('"mpmath"', '"sympy"', '"emcee"', '"corner"'): - assert package in hidden_imports.group("body") - - collect_loop = re.search(r"for\s+_pkg\s+in\s+\(([^)]*)\):", spec) - assert collect_loop is not None - for package in ('"mpmath"', '"sympy"', '"emcee"', '"corner"'): - assert package in collect_loop.group(1) + tree = ast.parse(spec, filename="DataLab.spec") + hiddenimports = next( + node.value + for node in ast.walk(tree) + if isinstance(node, ast.Assign) + and any( + isinstance(target, ast.Name) and target.id == "hiddenimports" + for target in node.targets + ) + ) + assert isinstance(hiddenimports, ast.List) + values = { + elt.value + for elt in hiddenimports.elts + if isinstance(elt, ast.Constant) and isinstance(elt.value, str) + } + assert {"mpmath", "sympy", "emcee", "corner"} <= values🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_implicit_packaging.py` around lines 13 - 21, Replace fragile regex checks with AST parsing of the spec source: parse the string variable spec using ast.parse, find the Assign node that sets the hiddenimports name (reference: hidden_imports variable and its regex) and evaluate its value with ast.literal_eval to get the actual list of module strings, then assert the expected packages ('mpmath','sympy','emcee','corner') are present; similarly find the For node whose target is _pkg (reference: collect_loop regex) and extract the iterable node (Tuple/List) from the AST and literal_eval that to verify the same package names, so tests validate Python values rather than source formatting.tests/test_implicit_d8_runner_regression.py (1)
76-81: ⚡ Quick winAvoid a hard 1-second wall-clock assertion in CI.
This check is likely to flap on slower or contended runners even when the planner/backend choice is correct. The strategy/backend assertions below already cover the functional regression; if you want a perf guard, keep it much looser or move it to a benchmark-style test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_implicit_d8_runner_regression.py` around lines 76 - 81, Remove the brittle wall-clock assertion in the test: delete or relax the line asserting time.perf_counter() - start < 1.0 in tests/test_implicit_d8_runner_regression.py; keep the two functional assertions on result.details["implicit_strategy"] == "observed_linear" and result.details["optimizer_backend"] == "mpmath_qr" intact, or if you want a perf guard, replace the hard 1.0s bound with a much looser threshold (or move it to a separate benchmark test) and document the threshold near the FitRunner().fit call.fitting/implicit_model.py (3)
857-871: ⚡ Quick winMissing
strict=Trueinzip()for linearity assertion.Line 861 uses
zip(basis_rows[row_index], trial_values)withoutstrict=True. If these sequences have different lengths, the computation would silently produce incorrect results.♻️ Proposed fix
linear = offsets[row_index] + mp.fsum( - coeff * value for coeff, value in zip(basis_rows[row_index], trial_values) + coeff * value for coeff, value in zip(basis_rows[row_index], trial_values, strict=True) )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@fitting/implicit_model.py` around lines 857 - 871, The zip in the linearity check inside the loop (where scope_base is computed via _observed_scope_for, actual via _eval_equation_with_params, and linear is computed using offsets[row_index] + mp.fsum(...)) must use zip(..., strict=True) to avoid silent truncation when basis_rows[row_index] and trial_values differ in length; update the generator expression that currently reads zip(basis_rows[row_index], trial_values) to zip(basis_rows[row_index], trial_values, strict=True) so a mismatched-length error is raised immediately.
306-316: 💤 Low valueUnused loop variable
target.The loop variable
targetat line 306 is not used within the loop body. Rename to_targetto indicate intentional non-use.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@fitting/implicit_model.py` around lines 306 - 316, The loop currently uses an unused variable named `target` in the comprehension "for row_index, target in enumerate(targets):" — rename `target` to `_target` to indicate intentional non-use and silence linters; update the loop header where `_observed_scope_for(definition, variable_data, targets, row_index)` and subsequent calls to `_eval_equation_with_params(definition, scope_base, ...)`, as well as uses of `parameter_state` and `basis_rows`, remain unchanged.
753-760: ⚡ Quick winMissing
strict=Trueinzip()calls for failure context.The
zip()calls at lines 755 and 759 could silently truncate ifvar_tuple/param_tuplelengths don't matchdefinition.x_variables/definition.parameters. While_validate_tuple_lengthsis called earlier in the call stack, addingstrict=Trueprovides defense-in-depth.♻️ Proposed fix
variable_values = { name: mp.nstr(value, 30) - for name, value in zip(definition.x_variables, var_tuple) + for name, value in zip(definition.x_variables, var_tuple, strict=True) } parameter_values = { name: mp.nstr(value, 30) - for name, value in zip(definition.parameters, param_tuple) + for name, value in zip(definition.parameters, param_tuple, strict=True) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@fitting/implicit_model.py` around lines 753 - 760, The zip() calls used to build variable_values and parameter_values can silently truncate mismatched iterables; add strict=True to both zip(...) invocations to enforce length equality and provide failure context (these are the comprehensions that zip definition.x_variables with var_tuple and definition.parameters with param_tuple), keeping _validate_tuple_lengths as defense-in-depth but ensuring the comprehensions raise on mismatch.extrapolation_methods/Data Extrapolation GUI.md (1)
79-87: 💤 Low valueFix heading hierarchy for consistent document structure.
Line 79 uses
#(h1) for section 2.3, but sections 2.1 and 2.2 use##(h2). This causes the###at line 87 to skip a level. Change line 79 to## 2.3 Explicit Model Selectionto match the other subsections.📝 Proposed fix
-# 2.3 Explicit Model Selection +## 2.3 Explicit Model Selection🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@extrapolation_methods/Data` Extrapolation GUI.md around lines 79 - 87, Change the heading "2.3 Explicit Model Selection" from a top-level H1 to an H2 to match sibling sections: replace the leading "#" with "##" for the line containing "2.3 Explicit Model Selection" so the subsequent "### Model Comparison" becomes a proper subheading under it.fitting/implicit_classifier.py (1)
182-190: 💤 Low valueBracket replacement may over-match array indexing syntax.
Line 190 replaces all
]with)unconditionally after converting function brackets. If an expression legitimately contains array indexing (e.g.,data[0]), this would corrupt it. For symbolic math expressions this is likely fine, but consider a more targeted replacement if array indexing is ever supported.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@fitting/implicit_classifier.py` around lines 182 - 190, The final unconditional replacement of "]" with ")" in _normalise_datalab_expression corrupts legitimate array indexing (e.g. data[0]); instead remove that blanket replace and ensure closing brackets are converted only when they match the function-opening replacements: change the loop to use a single targeted regex that rewrites patterns like \b(function_names)\s*\[(.*?)\] into \1(\2) (use non-greedy matching and DOTALL if needed) so only function call brackets are converted; update the loop that builds `updated` (which currently uses rf"\b({function_names})\s*\[") to perform the full pair replacement and then drop the final normalised.replace("]", ")").
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fd25da2d-8808-493a-8002-f14d19756628
📒 Files selected for processing (132)
CHANGELOG.mdDataLab.specREADME.mdapp_desktop/auto_fit_subprocess.pyapp_desktop/constants_editor.pyapp_desktop/fitting_input_normalization.pyapp_desktop/fitting_latex_writer.pyapp_desktop/formula_preview.pyapp_desktop/latex_highlighter.pyapp_desktop/log_scale_spinner.pyapp_desktop/main.pyapp_desktop/panels.pyapp_desktop/parallel_preferences.pyapp_desktop/parameter_table.pyapp_desktop/tutorial_overlay.pyapp_desktop/window.pyapp_desktop/window_data_mixin.pyapp_desktop/window_extrapolation_mixin.pyapp_desktop/window_fitting_mixin.pyapp_desktop/window_fitting_models_mixin.pyapp_desktop/window_fitting_residuals_mixin.pyapp_desktop/window_i18n_mixin.pyapp_desktop/window_latex_pdf_mixin.pyapp_desktop/workers_core.pyapp_desktop/workers_qt.pyapp_desktop/workspace_controller.pyapp_web/README_UPDATES.mdapp_web/blueprints/pages.pyapp_web/blueprints/sse.pyapp_web/formula_help_web.pyapp_web/logic/fitting.pyapp_web/logic/statistics.pyapp_web/openapi.pyapp_web/security.pyapp_web/server.pyapp_web/static/js/i18n.jsapp_web/streaming.pyapp_web/templates/fit.htmlbuild_mac_data_gui.shbuild_windows_data_gui.ps1cli/batch_config.pycli/main.pyconftest.pydatalab_latex/derivatives.pydatalab_latex/latex_tables_error_propagation.pydocs/ARCHITECTURE.mddocs/DATALAB_WEB_GUIDE.en.mddocs/DATALAB_WEB_GUIDE.mddocs/PROGRAM_FRAMEWORK.en.texdocs/PROGRAM_FRAMEWORK.texdocs/TEST_MATRIX.mddocs/desktop/fitting.en.mddocs/desktop/fitting.zh.mddocs/desktop/index.en.mddocs/desktop/index.zh.mddocs/superpowers/implicit_regression_evidence_matrix.mddocs/superpowers/plans/2026-05-29-datalab-fit-backend-ui-overhaul-implementation-plan.mddocs/superpowers/plans/2026-05-29-datalab-implicit-performance-auto-plan.mddocs/superpowers/specs/2026-05-29-datalab-fit-backend-ui-overhaul-design.mddocs/web/fitting.en.mddocs/web/fitting.zh.mddocs/web/index.en.mddocs/web/index.zh.mddocs/web/roadmap.en.mddocs/web/roadmap.zh.mddocs/web/theory.en.mddocs/web/theory.zh.mdexamples/workspaces/error-propagation.datalabexamples/workspaces/extrapolation.datalabexamples/workspaces/fitting.datalabexamples/workspaces/quantum-defect-implicit.datalabexamples/workspaces/statistics.datalabextrapolation_methods/Data Extrapolation GUI.mdfitting/__init__.pyfitting/hp_fitter.pyfitting/implicit_classifier.pyfitting/implicit_derivatives.pyfitting/implicit_model.pyfitting/implicit_planner.pyfitting/implicit_seed_hints.pyfitting/implicit_transforms.pyfitting/model_selector.pyfitting/plot_fitting.pyfitting/problem.pyfitting/report.pyfitting/runner.pyfitting/statistics.pyshared/parallel_config.pyshared/parsing.pyshared/symbolic_math.pyshared/uncertainty.pyshared/workspace_io.pytests/test_app_desktop_workers_core.pytests/test_auto_fit_removed.pytests/test_auto_fit_subprocess_orchestrator.pytests/test_cli_batch.pytests/test_clipboard_paste_parser.pytests/test_constants_editor.pytests/test_constants_editor_visibility.pytests/test_desktop_example_workspace_menu.pytests/test_desktop_implicit_model_ui.pytests/test_example_workspaces.pytests/test_fit_statistics.pytests/test_fitting_input_normalization.pytests/test_fitting_latex_writer.pytests/test_fitting_problem_boundary.pytests/test_fitting_runner_equivalence.pytests/test_fitting_runner_scipy_fallback.pytests/test_formula_preview_dialog.pytests/test_formula_preview_rendering.pytests/test_implicit_d8_runner_regression.pytests/test_implicit_derivatives.pytests/test_implicit_model.pytests/test_implicit_packaging.pytests/test_implicit_performance_regression.pytests/test_implicit_planner.pytests/test_implicit_scipy_backend.pytests/test_implicit_seed_hints.pytests/test_implicit_transforms.pytests/test_mcmc_gui_wiring.pytests/test_mcmc_pre_flight_health.pytests/test_openapi_spec.pytests/test_parallel_preferences.pytests/test_parameter_table.pytests/test_parameter_table_editor.pytests/test_symbolic_math.pytests/test_web_sse_fit_endpoint.pytests/test_workspace_auto_fit_migration.pytests/test_workspace_controller.pytests/test_workspace_implicit_round_trip.pytools/generate_example_workspaces.pyweb_requirements.txt
💤 Files with no reviewable changes (6)
- app_desktop/latex_highlighter.py
- app_web/openapi.py
- app_desktop/log_scale_spinner.py
- app_desktop/auto_fit_subprocess.py
- tests/test_auto_fit_subprocess_orchestrator.py
- shared/parallel_config.py
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app_desktop/fitting_input_normalization.py (1)
549-565:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
Δsigma-detection keyword is effectively dead.
lower_headerslowercases every header, so an uppercaseΔ(U+0394) header becomesδ(U+03B4). The keyword"Δ"on Line 551 is then matched against already-lowercased names and can never match. Delta-named uncertainty columns (common in physics datasets) silently fall through to theNoneresult. Use the lowercase form so the heuristic fires.🐛 Proposed fix
- keywords = ("sigma", "err", "error", "unc", "uncertainty", "Δ") + keywords = ("sigma", "err", "error", "unc", "uncertainty", "δ")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app_desktop/fitting_input_normalization.py` around lines 549 - 565, The heuristic never matches uppercase "Δ" because headers are lowercased into lower_headers; update the keyword list used in sigma/uncertainty detection so it contains the lowercase delta (e.g., "δ") or generate keywords by lowercasing them (modify the keywords tuple used in the loop such as keywords in the block that sets candidate_idx, or replace "Δ" with "δ") so comparisons against lower_headers and any(key in name for key in keywords) can succeed; ensure this change is applied where lower_headers, keywords, target_column, target_index, and candidate_idx are used in the shown block.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@app_desktop/fitting_input_normalization.py`:
- Around line 549-565: The heuristic never matches uppercase "Δ" because headers
are lowercased into lower_headers; update the keyword list used in
sigma/uncertainty detection so it contains the lowercase delta (e.g., "δ") or
generate keywords by lowercasing them (modify the keywords tuple used in the
loop such as keywords in the block that sets candidate_idx, or replace "Δ" with
"δ") so comparisons against lower_headers and any(key in name for key in
keywords) can succeed; ensure this change is applied where lower_headers,
keywords, target_column, target_index, and candidate_idx are used in the shown
block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 793444d9-7c6c-4d56-910c-50c00202f792
📒 Files selected for processing (34)
app_desktop/fitting_input_normalization.pyapp_desktop/panels.pyapp_desktop/window.pyapp_desktop/workspace_controller.pyapp_web/logic/fitting.pycli/batch_config.pycli/main.pydocs/superpowers/implicit_regression_evidence_matrix.mddocs/superpowers/specs/2026-05-29-datalab-fit-backend-ui-overhaul-design.mdexamples/workspaces/error-propagation.datalabexamples/workspaces/extrapolation.datalabexamples/workspaces/fitting.datalabexamples/workspaces/quantum-defect-implicit.datalabexamples/workspaces/statistics.datalabfitting/implicit_derivatives.pyfitting/implicit_model.pyfitting/problem.pyfitting/runner.pyfitting/statistics.pyshared/fitting_uncertainty.pyshared/uncertainty.pyshared/workspace_io.pytests/test_app_web_fitting_uncertainty.pytests/test_cli_batch.pytests/test_constants_editor.pytests/test_fit_statistics.pytests/test_fitting_input_normalization.pytests/test_fitting_problem_boundary.pytests/test_fitting_runner_equivalence.pytests/test_fitting_runner_scipy_fallback.pytests/test_implicit_model.pytests/test_shared_uncertainty.pytests/test_workspace_controller.pytests/test_workspace_io.py
✅ Files skipped from review due to trivial changes (2)
- docs/superpowers/implicit_regression_evidence_matrix.md
- docs/superpowers/specs/2026-05-29-datalab-fit-backend-ui-overhaul-design.md
🚧 Files skipped from review as they are similar to previous changes (10)
- tests/test_fitting_runner_equivalence.py
- shared/uncertainty.py
- tests/test_fit_statistics.py
- fitting/statistics.py
- tests/test_implicit_model.py
- cli/main.py
- app_desktop/workspace_controller.py
- app_web/logic/fitting.py
- fitting/implicit_model.py
- app_desktop/window.py
2 participants
Summary\n- Add and verify the general self-consistent implicit fitting backend with automatic route selection and stronger SciPy/mpmath fallback evidence.\n- Remove obsolete auto-fit UI/backend surfaces and stale backend toggles.\n- Unify fitting input normalization, constants/parameter handling, formula preview behavior, and example workspace coverage.\n- Harden worker payload/process-boundary tests and implicit cache identity evidence.\n\n## Verification\n- Focused implicit fitting, worker/process, GUI, workspace, and normalization tests were run across the implementation slices.\n- Latest focused checks include implicit SciPy/backend regression suites, cache identity tests, ruff, mypy, compileall, and branch source audits.\n\n## Release note\nThis PR is not a packaged release by itself. Release builds still require clean-source macOS/Windows packaging, signing/notarization or Authenticode verification, frozen-app smoke tests, and update manifest verification.
Summary by CodeRabbit
New Features
Improvements
Documentation