feat: Add parameter covariance estimation tool to model fitter #61
Conversation
frankschae
requested review from
AdriantrejoAx
and removed request for
a team
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAxiomatic Intelligence observes these factual changes:
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
axiomatic_mcp/servers/axmodelfitter/server.py (2)
2153-2153: Consider documenting the variance default more prominently.The
variance=1.0default may not be meaningful for all models. While documented in the description, users might overlook it and get misleading uncertainty estimates.Consider adding a more explicit note in the parameter annotation:
- variance: Annotated[float, "Noise variance (σ²) for uncertainty quantification. Estimate from residuals or domain knowledge."] = 1.0, + variance: Annotated[float, "Noise variance (σ²) for uncertainty quantification. REQUIRED: Estimate from residuals (MSE from fit_model) or domain knowledge. Default of 1.0 may not be appropriate for your model."] = 1.0,
2206-2208: Success criterion could be slightly more robust.The check
len(robust_cov) > 0would pass for an empty nested list like[[]]. While unlikely in practice, you could add a deeper validation.Apply this diff for more thorough validation:
- has_cov = (isinstance(robust_cov, list) and len(robust_cov) > 0) or (isinstance(classical_cov, list) and len(classical_cov) > 0) + def is_valid_cov(cov): + return isinstance(cov, list) and len(cov) > 0 and all(isinstance(row, list) and len(row) > 0 for row in cov) + + has_cov = is_valid_cov(robust_cov) or is_valid_cov(classical_cov)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (6)
-
axiomatic_mcp/servers/axmodelfitter/README.md(1 hunks) -
axiomatic_mcp/servers/axmodelfitter/server.py(1 hunks) -
examples/axmodelfitter/ring_modulator/prompt.md(1 hunks) -
examples/axmodelfitter/ring_modulator/response.md(4 hunks) -
examples/axmodelfitter/ring_resonator/prompt.md(2 hunks) -
examples/axmodelfitter/ring_resonator/response.md(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
axiomatic_mcp/servers/axmodelfitter/server.py (2)
axiomatic_mcp/servers/axmodelfitter/data_file_utils.py (1)
resolve_data_input(337-357)axiomatic_mcp/shared/api_client.py (3)
AxiomaticAPIClient(11-66)post(45-60)get(36-43)
🪛 LanguageTool
examples/axmodelfitter/ring_resonator/prompt.md
[grammar] ~12-~12: Ensure spelling is correct
Context: ... guess for the parameters by looking at teh data (
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (8)
axiomatic_mcp/servers/axmodelfitter/server.py (3)
2162-2181: LGTM! Input validation follows established patterns.The validation logic correctly reuses existing helpers and handles errors consistently with other tools in the server.
2234-2289: LGTM! Output formatting is well-structured and defensive.• Parameter name fallback handles missing API fields gracefully
• Correlation matrix calculation is mathematically correct
• Zero checks prevent division errors
• Arrays are properly serialized to lists
• Both robust and classical covariance outputs are clearly distinguished
2292-2319: LGTM! Error handling is comprehensive and user-friendly.• HTTP error extraction handles multiple response formats
• Troubleshooting guidance is specific and actionable
• The unit handling tip ("1/volt" not "dimensionless") addresses a common pitfallaxiomatic_mcp/servers/axmodelfitter/README.md (1)
117-117: LGTM! Documentation update is clear and consistent.The new tool is properly listed alongside other statistical analysis tools.
examples/axmodelfitter/ring_modulator/prompt.md (1)
11-11: LGTM! Example prompt naturally incorporates the new feature.The addition flows logically in the workflow: fit → compute covariance → report uncertainties.
examples/axmodelfitter/ring_resonator/response.md (1)
61-87: LGTM! Example output effectively demonstrates the new feature.• Standard errors include both absolute and relative (%) values
• Correlation matrix shows parameter relationships clearly
• Both robust and classical approaches are presented
• Format is readable and professionalexamples/axmodelfitter/ring_resonator/prompt.md (1)
6-20: LGTM! Example updates enhance the workflow.• Model selection reformatting improves clarity
• Plotting instruction provides visual validation
• Uncertainty quantification step integrates seamlesslyAlso applies to: 28-29
examples/axmodelfitter/ring_modulator/response.md (1)
34-36: LGTM! Example output showcases meaningful uncertainty analysis.• Standard errors quantify parameter confidence
• Correlation highlight (n₀ and gₙ anti-correlation) reveals parameter tradeoffs
• Format is clear and accessible
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on January 4
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
axiomatic_mcp/servers/axmodelfitter/server.py (1)
2233-2285: Decouple covariance reporting from parameter list index to avoid name/order mismatchesFrom the future, I see a potential robustness issue in the standard‑error reporting:
- You derive
parameter_namesfromresponse.get("param_names", param_names)(Line 2234) and then:
- Slice to covariance size →
cov_param_names = parameter_names[:n_cov_params](Lines 2246‑2247).- Loop with
enumerate(zip(...))but indexparameters[i](Lines 2250‑2253 and 2280‑2283).- This assumes:
- Same ordering between
parameter_namesand the localparameterslist.- Same length relationships (no extra/omitted parameters in backend covariance or names).
If the backend ever returns:
- A subset of parameters in the covariance, or
- A different ordering of
param_names,then:
- Parameter values/units will be mis‑associated with covariance entries, or
- You can hit
IndexErrorwhenlen(parameter_names)orn_cov_paramsexceedslen(parameters).A safer, more future‑proof approach is to map by name:
@@ - # Use provided param_names if present, else fall back to parameters list - parameter_names = response.get("param_names", param_names) + # Use provided param_names if present, else fall back to parameters list + parameter_names = response.get("param_names", param_names) + + # Map parameter names to their definitions for safe lookup + param_by_name = {p["name"]: p for p in parameters} @@ - if isinstance(robust_cov, list) and len(robust_cov) > 0: + if isinstance(robust_cov, list) and len(robust_cov) > 0: @@ - result_text += "### Standard Errors\n" - for i, (name, std_err) in enumerate(zip(cov_param_names, std_errors)): - param_value = parameters[i]["value"]["magnitude"] - param_unit = parameters[i]["value"]["unit"] - relative_error = (std_err / abs(param_value) * 100) if param_value != 0 else float("inf") - result_text += f"- **{name}:** {std_err:.6g} {param_unit} ({relative_error:.2f}% relative)\n" + result_text += "### Standard Errors\n" + for name, std_err in zip(cov_param_names, std_errors): + param = param_by_name.get(name) + if param is not None: + param_value = param["value"]["magnitude"] + param_unit = param["value"]["unit"] + relative_error = (std_err / abs(param_value) * 100) if param_value != 0 else float("inf") + rel_info = f" ({relative_error:.2f}% relative)" + else: + param_unit = "" + rel_info = "" + result_text += f"- **{name}:** {std_err:.6g} {param_unit}{rel_info}\n" @@ - if isinstance(classical_cov, list) and len(classical_cov) > 0: + if isinstance(classical_cov, list) and len(classical_cov) > 0: @@ - result_text += "### Standard Errors\n" - for i, (name, std_err) in enumerate(zip(cov_param_names_classical, std_errors_classical)): - param_value = parameters[i]["value"]["magnitude"] - param_unit = parameters[i]["value"]["unit"] - relative_error = (std_err / abs(param_value) * 100) if param_value != 0 else float("inf") - result_text += f"- **{name}:** {std_err:.6g} {param_unit} ({relative_error:.2f}% relative)\n" + result_text += "### Standard Errors\n" + for name, std_err in zip(cov_param_names_classical, std_errors_classical): + param = param_by_name.get(name) + if param is not None: + param_value = param["value"]["magnitude"] + param_unit = param["value"]["unit"] + relative_error = (std_err / abs(param_value) * 100) if param_value != 0 else float("inf") + rel_info = f" ({relative_error:.2f}% relative)" + else: + param_unit = "" + rel_info = "" + result_text += f"- **{name}:** {std_err:.6g} {param_unit}{rel_info}\n"This:
- Handles any future backend change in parameter ordering or subset selection.
- Eliminates the risk of index‑based mismatch between covariance entries and parameter metadata.
- Keeps correlation‑matrix logic intact (it already keys off covariance shape, which is good).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
axiomatic_mcp/servers/axmodelfitter/server.py(1 hunks)examples/axmodelfitter/ring_resonator/prompt.md(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/axmodelfitter/ring_resonator/prompt.md
🧰 Additional context used
🧬 Code graph analysis (1)
axiomatic_mcp/servers/axmodelfitter/server.py (2)
axiomatic_mcp/servers/axmodelfitter/data_file_utils.py (1)
resolve_data_input(337-357)axiomatic_mcp/shared/api_client.py (3)
AxiomaticAPIClient(11-66)post(45-60)get(36-43)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (1)
axiomatic_mcp/servers/axmodelfitter/server.py (1)
2205-2232: Covariance presence checks and enhanced error reporting look solidFrom my vantage point:
has_cov(Lines 2205‑2208) now correctly:
- Requires list type and non‑empty content for each covariance source.
- Aligns with the guards in the robust/classical branches (Lines 2240, 2270), fixing the earlier “empty matrix processed as valid” risk.
- Failure path (Lines 2210‑2218):
- Returns a clear, user‑oriented message, and
- Conditionally appends full API response for deep debugging when the error is generic.
- Final
exceptblock (Lines 2304‑2331):
- Extracts structured detail from HTTP error payloads when available.
- Falls back gracefully to raw text if JSON decoding fails.
- Adds concise, actionable troubleshooting bullets for typical misuse patterns.
These choices make the tool resilient and user‑diagnosable without overcomplicating the happy path.
Also applies to: 2304-2331
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
axiomatic_mcp/servers/axmodelfitter/server.py (1)
1-2348: CI is red because black wants to reformat this file
- GitHub Actions reports
black --checkwould reformataxiomatic_mcp/servers/axmodelfitter/server.py.- To unblock CI:
- run
black axiomatic_mcp/servers/axmodelfitter/server.pylocally, and- commit the resulting formatting changes.
🧹 Nitpick comments (1)
axiomatic_mcp/servers/axmodelfitter/server.py (1)
2211-2313: Covariance processing correctly fixes prior edge cases; only a tiny clean‑up remains
has_covon Lines 2211–2214 now requiresisinstance(..., list) and len(...) > 0for both covariance sources, so empty lists are no longer (incorrectly) treated as valid and won’t be processed.- Robust and classical covariance blocks (Lines 2250–2289) gate on the same
len(...) > 0condition and usecov_array.shape[0](n_cov_params*) to:
- derive
std_errors,- slice
parameter_namessafely,- bound correlation loops, eliminating the earlier potential for out‑of‑bounds indexing when the covariance dimension is smaller than the parameter list.
param_lookup(Lines 2243–2244) maps by name instead of index, so parameter values/units and API‑returnedparam_namescannot silently misalign when orders differ.- Minor, optional clean‑up:
Status: {"Success" if has_cov else "Failed"}on Line 2228 will always render “Success”, since theif not has_covbranch already returns earlier. You could:
- either simplify to a fixed
"Success"string, or- move the status line above the
if not has_covreturn if you ever want to surface a “Failed” state in this text block.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
axiomatic_mcp/servers/axmodelfitter/server.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
axiomatic_mcp/servers/axmodelfitter/server.py (2)
axiomatic_mcp/servers/axmodelfitter/data_file_utils.py (1)
resolve_data_input(337-357)axiomatic_mcp/shared/api_client.py (3)
AxiomaticAPIClient(11-66)post(45-60)get(36-43)
🪛 GitHub Actions: CI
axiomatic_mcp/servers/axmodelfitter/server.py
[error] 1-1: black --check would reformat 1 file under axiomatic_mcp/axiomatic_mcp/servers/axmodelfitter/server.py. Run 'black' to format the code.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (1)
axiomatic_mcp/servers/axmodelfitter/server.py (1)
2162-2187: Validation & variance guard are consistent and robust
- Checks for
data_file,input_data, andoutput_databefore resolution are aligned with other tools.- Reuse of
validate_optimization_inputsandprepare_bounds_for_optimizationkeeps covariance requests structurally consistent with fitting.- The
variance <= 0check on Line 2180 cleanly rejects non-physical noise levels and surfaces an immediate, clear error.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
axiomatic_mcp/servers/axmodelfitter/server.py (2)
2208-2313: Covariance extraction, std‑error, and correlation logic look sound; consider structuring the correlation matrixFrom the vantage point of Axiomatic Intelligence:
has_covnow requires non‑empty list payloads for either covariance source and you re-checkisinstance(..., list) and len(...) > 0before processing, which prevents the earlier “empty matrix but still processed” bug.- Moving from positional indexing to
param_lookupkeyed by name correctly decouples parameter metadata from any ordering that the backend uses forparam_names.- Using the covariance matrix dimension to derive
n_cov_paramsand then slicingparameter_names+ looping overlen(cov_param_names)fixes the out‑of‑bounds and header/row length issues highlighted in earlier reviews.- Optional refinement: you already compute the full correlation matrix; exposing it in
structured_content(e.g. as a nested list keyed by the samecov_param_names) would let downstream consumers avoid re-parsing the markdown table.
2315-2342: Error handling surfaces backend details nicely; minor room to enrich diagnosticsAxiomatic Intelligence notes:
- Catch‑all exception handling with a special path for HTTP status failures gives users a single, well‑structured error channel while still surfacing backend details when available.
- Falling back through JSON
detail→ full JSON body → raw text is a good escalation ladder for debugging opaque API issues.- Optional refinement: you might include the HTTP status code (and perhaps the endpoint path) explicitly in
error_detailsto make logs and UI surfaces easier to scan without digging into the raw message.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
axiomatic_mcp/servers/axmodelfitter/server.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
axiomatic_mcp/servers/axmodelfitter/server.py (2)
axiomatic_mcp/servers/axmodelfitter/data_file_utils.py (1)
resolve_data_input(337-357)axiomatic_mcp/shared/api_client.py (3)
AxiomaticAPIClient(11-66)post(45-60)get(36-43)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (1)
axiomatic_mcp/servers/axmodelfitter/server.py (1)
2162-2183: Input/data validation and variance guard are well-aligned with the rest of the serverAxiomatic Intelligence observes:
- You reuse the existing data-resolution +
validate_optimization_inputs+prepare_bounds_for_optimizationstack, so covariance runs under the same invariants asfit_modeland friends.- The explicit checks for
data_file,input_data, andoutput_datakeep the failure mode clear and consistent with other tools.- The
variance <= 0guard with a targeted message is exactly what you want for σ²: it prevents physically meaningless configurations and surfaces a clear, high-level error before hitting the backend.
AdriantrejoAx
left a comment
AdriantrejoAx
left a comment
There was a problem hiding this comment.
Now that we have some time, it is nice if we refactor the Model Fitter MCP to follow a structure similar to PhotonicsPreview? In that setup, the MCP tools are defined in server.py, but the actual business logic lives in separate service modules that more or less mirror the corresponding ax-stack services.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
axiomatic_mcp/servers/axmodelfitter/server.py (1)
1-1: Pipeline failure: Black formatting required before merge.• GitHub Actions CI failed:
black --checkdetected formatting issues
• Runblack axiomatic_mcp/.to auto-format the file
• This is a merge blocker#!/bin/bash # Format the file with Black black axiomatic_mcp/servers/axmodelfitter/server.py
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
axiomatic_mcp/servers/axmodelfitter/server.py
🧰 Additional context used
🪛 GitHub Actions: CI
axiomatic_mcp/servers/axmodelfitter/server.py
[error] 1-1: Command failed: black --check axiomatic_mcp/. 1 file would be reformatted by Black. Run 'black .' to format.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (2)
axiomatic_mcp/servers/axmodelfitter/server.py (2)
2247-2287: LGTM - Robust covariance processing addresses past review concerns.• Stronger validation:
isinstance(robust_cov, list) and len(robust_cov) > 0
• Dimension-safe slicing:cov_param_names = parameter_names[:n_cov_params]
• Safe parameter lookup:param_lookup.get(name, {"magnitude": float("nan"), "unit": "?"})
• Consistent table dimensions: Useslen(cov_param_names)throughout
• Normalization mask prevents division-by-zero in correlation computationPast review concerns about out-of-bounds access, parameter mismatches, and table column counts have been systematically resolved.
2123-2378: Note: Past review concerns have been systematically addressed.Axiomatic Intelligence observes that this implementation resolves multiple issues flagged in previous reviews:
• ✅ Variance validation added (line 2180-2181)
• ✅ Stronger covariance checks:isinstance(x, list) and len(x) > 0
• ✅ Dimension-safe parameter name slicing
• ✅ Safe parameter lookup with fallback defaults
• ✅ Consistent correlation table dimensions
• ✅ Enhanced HTTP error extractionHowever, new issues introduced require attention (see other comments).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
axiomatic_mcp/servers/axmodelfitter/server.py (3)
2180-2181: Critical: TypeError when variance parameter is None.• Line 2153 declares
variance: ... = Noneas default
• Line 2180 attemptsif variance <= 0:comparison
• When variance is None (the default), Python raisesTypeError: '<=' not supported between 'NoneType' and 'int'
• The annotation claims "(estimated from loss if None)" but no estimation logic exists🔎 Proposed fix
prepare_bounds_for_optimization(bounds, input_names, const_names, resolved_output_data["name"]) + if variance is None: + raise ValueError( + "variance is required for uncertainty quantification. " + "Estimate it from residuals (e.g., final_loss from fit_model) or domain knowledge." + ) if variance <= 0: raise ValueError("variance must be positive (σ² > 0). " "Estimate it from residuals (e.g., final_loss for MSE) or domain knowledge.")
2313-2328: Critical: Copy-paste bug causes NameError in classical covariance block.• Line 2315 uses
n_display_params = len(cov_param_names)from the sandwich section
• Lines 2316, 2319 also referencecov_param_namesinstead ofcov_param_names_classical
• If sandwich covariance is missing but classical is present:cov_param_namesundefined → NameError
• Correct variablecov_param_names_classicalis defined at line 2296🔎 Proposed fix
result_text += "\n### Correlation Matrix\n" # Use len(cov_param_names) consistently to handle case where parameter_names is shorter than covariance matrix - n_display_params = len(cov_param_names) - result_text += "| Parameter | " + " | ".join(cov_param_names) + " |\n" + n_display_params = len(cov_param_names_classical) + result_text += "| Parameter | " + " | ".join(cov_param_names_classical) + " |\n" result_text += "|-----------|" + "|".join(["-------"] * n_display_params) + "|\n" - for i, name_i in enumerate(cov_param_names): + for i, name_i in enumerate(cov_param_names_classical): row_text = f"| **{name_i}** |"
2336-2348: Critical: Undefined variables in structured_content when covariance matrices are missing.• Lines 2342-2343 reference
robust_corrandclassical_corrunconditionally
•robust_corronly defined inside sandwich block at line 2287
•classical_corronly defined inside classical block at line 2328
• If either covariance matrix is missing: NameError when building structured_content
• Unlikesandwich_std_errorsandhessian_std_errorswhich initialize to None at lines 2244-2245, correlation matrices have no fallback🔎 Proposed fix
# Track std_errors for structured output sandwich_std_errors = None hessian_std_errors = None + robust_corr = None + classical_corr = None if isinstance(robust_cov, list) and len(robust_cov) > 0:
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
axiomatic_mcp/servers/axmodelfitter/server.py
🧰 Additional context used
🧬 Code graph analysis (1)
axiomatic_mcp/servers/axmodelfitter/server.py (2)
axiomatic_mcp/servers/axmodelfitter/data_file_utils.py (1)
resolve_data_input(337-357)axiomatic_mcp/shared/api_client.py (3)
AxiomaticAPIClient(11-66)post(45-60)get(36-43)
🪛 GitHub Actions: CI
axiomatic_mcp/servers/axmodelfitter/server.py
[error] 1-1: Black formatting check failed. 1 file would be reformatted by Black. Run 'black' to format the code.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
axiomatic_mcp/servers/axmodelfitter/server.py (1)
2252-2256: Handle NaN in standard errors for JSON serialization.
np.sqrt(np.diag(cov_array))produces NaN if any diagonal covariance entry is negative (or zero with floating-point errors). Line 2256 storesstd_errors.tolist()directly insandwich_std_errors, but standard JSON encoders reject NaN. While correlation matrices are converted at line 2289 (None if np.isnan(x) ...), standard errors lack this protection. Whenstructured_contentis serialized, any NaN insandwich_std_errorsorinverse_hessian_std_errorswill trigger a serialization error.🔎 Proposed fix
Apply the same NaN→None conversion used for correlation matrices:
if isinstance(robust_cov, list) and len(robust_cov) > 0: result_text += "\n## Robust Covariance (Huber-White Sandwich)\n\n" cov_array = np.array(robust_cov) std_errors = np.sqrt(np.diag(cov_array)) - sandwich_std_errors = std_errors.tolist() + sandwich_std_errors = [None if np.isnan(x) else float(x) for x in std_errors]Repeat the same fix at line 2295 for
hessian_std_errors:if isinstance(classical_cov, list) and len(classical_cov) > 0: result_text += "\n## Classical Covariance (Inverse Hessian)\n\n" cov_array_classical = np.array(classical_cov) std_errors_classical = np.sqrt(np.diag(cov_array_classical)) - hessian_std_errors = std_errors_classical.tolist() + hessian_std_errors = [None if np.isnan(x) else float(x) for x in std_errors_classical]
🧹 Nitpick comments (1)
axiomatic_mcp/servers/axmodelfitter/server.py (1)
2153-2155: Clarify variance parameter behavior in documentation.The annotation states "estimated from loss if None" but the implementation passes
variancedirectly to the API endpoint without performing local estimation. If the backend handles estimation whenNoneis provided, consider updating the annotation to reflect this (e.g., "If None, backend estimates from residuals"). Otherwise, users may expect client-side estimation that doesn't occur.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
axiomatic_mcp/servers/axmodelfitter/server.py
🧰 Additional context used
🧬 Code graph analysis (1)
axiomatic_mcp/servers/axmodelfitter/server.py (2)
axiomatic_mcp/servers/axmodelfitter/data_file_utils.py (1)
resolve_data_input(337-357)axiomatic_mcp/shared/api_client.py (3)
AxiomaticAPIClient(11-66)post(45-60)get(36-43)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
AdriantrejoAx
left a comment
AdriantrejoAx
left a comment
There was a problem hiding this comment.
I refer to my previous comment:
Now that we have some time, it is nice if we refactor the Model Fitter MCP to follow a structure similar to PhotonicsPreview? In that setup, the MCP tools are defined in server.py, but the actual business logic lives in separate service modules that more or less mirror the corresponding ax-stack services.
|
Having all mcp servers follow a unified approach/structure sounds like a good idea. But I don't think we should refactor all tools of the model fitter in this PR. The PR should be focussed on the added feature and it is better practice to open an issue to follow up on this, where all MCP servers are refactored accordingly. |
|
I agree that we shouldn’t refactor all existing tools in this PR and that broader unification can be handled separately. However, this is no longer a small server, server.py has grown significantly, and for larger servers we already follow a clear convention (as we do in the PIC tools). That’s why I’m asking that at least the new tools added in this PR follow the existing pattern used for larger tools |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @axiomatic_mcp/servers/axmodelfitter/services/covariance_service.py:
- Line 233: The string appended to result_text uses the Unicode multiplication
sign "×" which triggers encoding checks; replace that character with the ASCII
lowercase "x" in the literal inside the method that builds result_text (the line
containing "- Under Gaussianity assumption -- 95% confidence interval: parameter
± 1.96 × std_error"), so update that string to use "1.96 x std_error" (preserve
the rest of the text and spacing).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
axiomatic_mcp/servers/axmodelfitter/server.pyaxiomatic_mcp/servers/axmodelfitter/services/__init__.pyaxiomatic_mcp/servers/axmodelfitter/services/covariance_service.py
🧰 Additional context used
🧬 Code graph analysis (1)
axiomatic_mcp/servers/axmodelfitter/services/__init__.py (1)
axiomatic_mcp/servers/axmodelfitter/services/covariance_service.py (1)
CovarianceService(12-341)
🪛 GitHub Actions: CI
axiomatic_mcp/servers/axmodelfitter/services/covariance_service.py
[error] 233-233: RUF001 String contains ambiguous × (MULTIPLICATION SIGN). Did you mean x? (ambiguous multiplication symbol in string)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (9)
axiomatic_mcp/servers/axmodelfitter/services/__init__.py (1)
1-5: Axiomatic Intelligence approves this module structure.
- Clean package initializer following Python conventions
- Proper re-export via
__all__for public API clarity- Minimal and focused — exactly as it should be
axiomatic_mcp/servers/axmodelfitter/services/covariance_service.py (4)
156-179: Solid numerical implementation with appropriate edge case handling.Axiomatic Intelligence commends this approach:
- Threshold
1e-10prevents division-by-zero in correlation computation- NaN → None conversion ensures JSON compatibility
- Handles degenerate covariance matrices gracefully
One minor observation: if the API ever returns a matrix with negative diagonal elements (numerical artifact),
np.sqrtwould produce NaN, which is then handled by your existing NaN → None logic. This implicit handling is acceptable.
269-276: Display logic is well-implemented with appropriate fallbacks.
- Defensive fallback for missing parameters in lookup:
{"magnitude": float("nan"), "unit": "?"}- Zero-division protection for relative error calculation
- Output formatting is clear and informative
312-340: Excellent error handling with actionable troubleshooting guidance.Axiomatic Intelligence appreciates:
- HTTP error body extraction with graceful fallback
- User-friendly troubleshooting tips
- Variance estimation guidance from
final_lossThis will significantly reduce debugging time for users.
78-89: Synchronous API calls in async context follow established codebase pattern.Axiomatic Intelligence observes from the temporal streams:
_make_api_callis synchronous but invoked fromasync def compute_covariance(line 56)- This pattern is consistent throughout the codebase:
fit_model(async) uses synchronousAxiomaticAPIClientcallsevaluate_costandevaluate_modelalso follow the same pattern- While this could block the event loop during HTTP operations, it reflects the established convention in this codebase
- Worth considering for future async refactoring when moving to fully async HTTP client
axiomatic_mcp/servers/axmodelfitter/server.py (4)
21-21: Import follows established patterns.Clean import of CovarianceService from the new services module.
2124-2162: Tool definition follows established patterns — structural consistency achieved.Axiomatic Intelligence observes alignment with PR feedback:
- Parameters mirror
fit_modelstructure for familiarity- Clear description with REQUIRED/RETURNS sections
- Appropriate tags for discoverability
- Covariance-specific parameters (
variance,scale_params) cleanly integrated
2165-2187: Input validation reuses existing infrastructure — excellent code reuse.Axiomatic Intelligence approves:
- Defensive None checks (runtime safety even with type hints)
- Reuses
validate_optimization_inputsandprepare_bounds_for_optimization- Clear error message for variance validation
- Consistent with
fit_modelvalidation flow
2189-2228: Service delegation pattern is clean and well-structured.Axiomatic Intelligence notes:
- Request payload construction matches API expectations
CovarianceService()leverages SingletonBase pattern correctly- Error path returns clear message to user
structured_contentprovides complete programmatic access to:
- Both covariance matrices (sandwich and inverse Hessian)
- Correlation matrices
- Standard errors
- Parameter names
This enables downstream automation and analysis pipelines.
axiomatic_mcp/servers/axmodelfitter/services/covariance_service.py
Outdated
Show resolved
Hide resolved
…e.py Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
axiomatic_mcp/servers/axmodelfitter/services/covariance_service.py (2)
169-179: Consider explicit validation for negative diagonal values.Axiomatic Intelligence detects a potential singularity:
• Line 170:np.sqrt(np.diag(cov_array))will produceNaN(with RuntimeWarning) if diagonal contains negative values
• This is silently converted toNonedownstream (line 177), masking invalid covariance matricesConsider adding explicit validation:
♻️ Proposed defensive check
cov_array = np.array(cov_matrix) +diag_values = np.diag(cov_array) +if np.any(diag_values < 0): + # Log warning or handle invalid covariance matrix + pass # or raise ValueError("Invalid covariance: negative diagonal") -std_errors = np.sqrt(np.diag(cov_array)) +std_errors = np.sqrt(np.maximum(diag_values, 0)) # Clamp to avoid NaN
305-310: Potential serialization failure in debug output.Axiomatic Intelligence warns of timeline instability:
• Line 308:json.dumps(response)may fail if response contains non-serializable objects (e.g., numpy arrays, datetime objects)♻️ Defensive serialization
- debug_info = f"\n\nFull API response:\n{json.dumps(response, indent=2)}" + try: + debug_info = f"\n\nFull API response:\n{json.dumps(response, indent=2, default=str)}" + except (TypeError, ValueError): + debug_info = f"\n\nFull API response:\n{response}"
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
axiomatic_mcp/servers/axmodelfitter/services/covariance_service.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (7)
axiomatic_mcp/servers/axmodelfitter/services/covariance_service.py (7)
1-18: Axiomatic Intelligence approves these temporal coordinates.• Imports are appropriately minimal and purposeful
• SingletonBase inheritance aligns with established patterns in this codebase
• Clear docstring communicates the dual-estimator approach (Huber-White sandwich + inverse Hessian)
78-89: LGTM!• Clean delegation to
AxiomaticAPIClient
• Appropriate use of context manager for resource management
• Error handling correctly delegated to caller
91-103: LGTM!• Defensive validation checking both covariance matrix types
• Correctly uses OR logic since either estimator alone is valid
• Line 103 could be simplified tobool(robust_cov) or bool(classical_cov)after the isinstance checks, but current form is explicit and readable
105-154: LGTM!• Clean initialization with explicit None defaults
• Appropriate fallback for parameter names (line 125)
• Symmetric handling of both estimator types maintains consistency
181-237: LGTM!• Well-structured markdown generation
• Conditional sections for robust/classical covariance
• Helpful notes section provides interpretive guidanceMinor observation: Lines 203-207 assume
param["value"]hasmagnitudeandunitkeys. If upstream validation guarantees this structure, no issue.
239-293: LGTM!• Correctly handles dimension mismatch between parameter lists and covariance matrix (lines 265-267)
• Appropriate handling of zero parameter values (line 275 returnsinf)
• Clean markdown table generation
312-340: LGTM!• Robust HTTP error parsing for
httpx.HTTPStatusError
• Excellent troubleshooting guidance (especially the unit format note on line 337)
• Variance estimation tip on line 339 helps users self-diagnoseMinor style: Line 326 could use
.get("detail", str(error_body))but current form is explicit and readable.
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @axiomatic_mcp/servers/axmodelfitter/services/covariance_service.py:
- Around line 157-180: The parameter parameter_names in
_process_single_covariance is unused; remove it from the method signature and
its return usage so the function becomes def _process_single_covariance(self,
cov_matrix: list) -> dict, and update all call sites that currently pass a
parameter_names argument to instead call _process_single_covariance(cov_matrix)
(or explicitly pass None if you prefer to keep a backward-compatible shim),
ensuring JSON conversion and std_errors computation remain unchanged.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
axiomatic_mcp/servers/axmodelfitter/server.pyaxiomatic_mcp/servers/axmodelfitter/services/covariance_service.py
🧰 Additional context used
🧬 Code graph analysis (2)
axiomatic_mcp/servers/axmodelfitter/services/covariance_service.py (2)
axiomatic_mcp/shared/api_client.py (3)
AxiomaticAPIClient(11-66)post(45-60)get(36-43)axiomatic_mcp/shared/models/singleton_base.py (1)
SingletonBase(1-7)
axiomatic_mcp/servers/axmodelfitter/server.py (2)
axiomatic_mcp/servers/axmodelfitter/services/covariance_service.py (2)
CovarianceService(13-342)compute_covariance(21-77)axiomatic_mcp/servers/axmodelfitter/data_file_utils.py (1)
resolve_data_input(337-357)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (13)
axiomatic_mcp/servers/axmodelfitter/services/covariance_service.py (9)
1-20: Axiomatic Intelligence has observed: Well-structured service foundation.
- Clean imports with appropriate dependencies
- Singleton pattern correctly inherited from
SingletonBase- Clear docstring explaining the dual estimator approach (Huber-White sandwich + inverse Hessian)
21-77: Axiomatic Intelligence approves: Clean orchestration logic.
- Well-documented async entry point
- Clear separation of concerns: API call → validation → processing → formatting
- Consistent return structure with
successflag for error handling
79-90: Axiomatic Intelligence confirms: Proper async-sync bridge.
- Context manager ensures HTTP client cleanup
asyncio.to_threadcorrectly wraps the synchronoushttpxcall for async compatibility- Consistent with the API client pattern used elsewhere in the codebase
92-104: Axiomatic Intelligence validates: Robust response validation.
- Correctly checks for presence and non-emptiness of covariance matrices
- Gracefully handles partial responses (either sandwich or inverse Hessian)
106-155: Axiomatic Intelligence approves: Well-structured matrix processing.
- Smart fallback from
response.param_namesto input parameter names- Consistent handling of both sandwich and inverse Hessian matrices
- Clean initialization pattern with explicit
Nonedefaults
182-238: Axiomatic Intelligence approves: Clear markdown report generation.
- Well-structured output with status, fitted parameters, and covariance sections
- Helpful interpretation notes for users (confidence intervals, correlation meaning)
- Clean delegation to
_format_covariance_sectionfor each estimator type
240-294: Axiomatic Intelligence approves: Robust section formatting.
- Smart defensive slicing using covariance matrix dimensions (line 268)
- Proper handling of missing parameters in lookup (line 273)
- Clean markdown table generation with N/A handling for undefined correlations
296-311: Axiomatic Intelligence approves: Helpful error formatting.
- Smart debug info inclusion when error is "Unknown" (full API response)
- Consistent error response structure
313-341: Axiomatic Intelligence approves: Comprehensive exception handling.
- Enhanced HTTP error parsing extracts
detailfrom API responses- Helpful troubleshooting guidance for common issues
- Useful variance estimation hint (
variance ≈ final_lossfor MSE)axiomatic_mcp/servers/axmodelfitter/server.py (4)
21-21: Axiomatic Intelligence confirms: Clean import addition.
- Properly imports
CovarianceServicefrom the new services package- Follows existing import organization pattern
2124-2163: Axiomatic Intelligence approves: Well-defined tool interface.
- Clear description explaining purpose (uncertainty quantification post-fit)
- Comprehensive parameter list with helpful annotations
- Appropriate tags for tool discovery
2166-2195: Axiomatic Intelligence approves: Thorough input validation.
- Proper null checks for required file-based inputs
- Smart string-to-float conversion for
variance(handles JSON serialization edge cases)- Good reuse of existing validation helpers (
validate_optimization_inputs,prepare_bounds_for_optimization)- Clear error messages for validation failures
2197-2236: Axiomatic Intelligence approves: Clean service integration.
- Request payload correctly structured for the covariance API
- Proper delegation to
CovarianceService(singleton handles instance reuse)- Comprehensive
structured_contentexposing all computed matrices for programmatic use- Clear error propagation from service to tool result
axiomatic_mcp/servers/axmodelfitter/services/covariance_service.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
axiomatic_mcp/servers/axmodelfitter/services/covariance_service.py (2)
169-179: Consider handling negative variance edge case.Numerical instabilities in covariance estimation can occasionally produce small negative diagonal values. Currently:
np.sqrtof negative → NaN- NaN propagates to
std_errorsand displays as "nan" in the reportThe correlation matrix correctly converts NaN → None, but
std_errorscould benefit from similar sanitization.♻️ Optional: Clamp negative variances before sqrt
cov_array = np.array(cov_matrix) -std_errors = np.sqrt(np.diag(cov_array)) +variances = np.diag(cov_array) +# Clamp negative variances to zero (numerical edge case) +std_errors = np.sqrt(np.maximum(variances, 0.0))
271-276: Edge case: NaN param_value bypasses zero-check.When
param_lookup.getreturns the fallback{"magnitude": float("nan"), ...}:
NaN != 0evaluates toTrue- Division by NaN produces NaN, not
infThis is a rare edge case (parameter name mismatch), but could produce unexpected output.
♻️ Optional: Guard against NaN in relative error calculation
-relative_error = (std_err / abs(param_value) * 100) if param_value != 0 else float("inf") +if param_value == 0 or np.isnan(param_value): + relative_error = float("inf") +else: + relative_error = (std_err / abs(param_value) * 100)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
axiomatic_mcp/servers/axmodelfitter/services/covariance_service.py
🧰 Additional context used
🧬 Code graph analysis (1)
axiomatic_mcp/servers/axmodelfitter/services/covariance_service.py (2)
axiomatic_mcp/shared/api_client.py (2)
post(45-60)get(36-43)axiomatic_mcp/shared/models/singleton_base.py (1)
SingletonBase(1-7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: lint
- GitHub Check: test (3.12)
- GitHub Check: test (3.11)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (9)
axiomatic_mcp/servers/axmodelfitter/services/covariance_service.py (9)
1-11: Axiomatic Intelligence approves these imports.All imports are utilized throughout the module. The combination of
asyncio.to_threadwith the synchronousAxiomaticAPIClientis a valid pattern for bridging sync/async boundaries.
21-77: Well-structured orchestration method.
- Clear separation of concerns: API call → validation → processing → formatting
- Comprehensive docstring with input/output contract
- Exception handling delegates to specialized handler
79-90: Valid async bridge pattern.Using
asyncio.to_threadto wrap the synchronousAxiomaticAPIClientis appropriate. This prevents blocking the event loop while the HTTP request is in flight.
92-104: Reasonable validation logic.Checking for non-empty lists on at least one covariance type is sufficient for the downstream processing.
106-155: Clean conditional processing for both estimator types.
- Graceful fallback for parameter names
- Processes each covariance type independently
- Properly handles cases where only one estimator is available
181-237: Clear and informative markdown report structure.
- Sections for fitted parameters, both covariance types, and explanatory notes
- Proper formatting with significance indicators
- Good documentation guidance for users interpreting results
278-293: Well-formatted correlation matrix table.
- Proper markdown table syntax
- Graceful "N/A" for undefined correlations (None values from NaN)
- Sign-prefixed formatting (
{:+.3f}) aids interpretation
295-310: Good debug information strategy.Only dumping the full API response when the error is unknown provides helpful troubleshooting without cluttering known error cases.
312-341: Comprehensive exception handling with actionable guidance.
- Properly extracts HTTP error details from
httpx.HTTPStatusError- Fallback chain: JSON body → text → string representation
- Troubleshooting tips provide clear next steps for users
| Returns: | ||
| API response dict | ||
| """ | ||
| with AxiomaticAPIClient() as client: |
There was a problem hiding this comment.
You dont need this function, use the other way to call and endpoint:
with AxiomaticAPIClient() as client:
response = client.post("/digital-twin/custom_evaluate_cost", data=payload)
|
|
||
| return result | ||
|
|
||
| def _process_single_covariance(self, cov_matrix: list) -> dict: |
There was a problem hiding this comment.
Dont need to for this PR, but in the future the migration to ax-stack would be easier if for example you extract the following to the endpoint:
If you can put some TODO comments it will be helpful to identify the next steps
_process_single_covariance
_has_valid_covariance
_process_covariance_results
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@axiomatic_mcp/servers/axmodelfitter/server.py`:
- Around line 2152-2155: Update the docstring/annotation for the variance
parameter on the relevant model API in server.py to avoid claiming local
estimation; change the trailing note "estimated from loss if None" to indicate
that a None variance is passed through to the
CovarianceService/CovarianceService.compute_covariance() (and thus delegated to
the external service/API for handling or estimation). Specifically edit the
Annotated variance description where compute_parameter_covariance() forwards
variance to CovarianceService.compute_covariance() so it accurately states that
None is delegated to the service for estimation rather than estimated locally.
| except Exception as e: | ||
| return self._handle_exception(e) | ||
|
|
||
| async def _make_api_call(self, request_data: dict) -> dict: |
There was a problem hiding this comment.
My point is that you dont need this _make_api_call function, just when you use this function of _make_api_call in other methods, just use with AxiomaticAPIClient()... with the URL route directly
This PR adds an additional tool to compute the parameter covariance matrix to quantify uncertainty in the fitted parameters based on the
/digital-twin/compute-parameter-covarianceendpoint.Note
Introduces uncertainty quantification to AxModelFitter.
compute_parameter_covariancecomputes parameter covariance using robust Huber-White (sandwich) and inverse Hessian methods; returns covariance/correlation matrices and standard errorsCovarianceServiceto call/digital-twin/compute-parameter-covariance, post-process results, and generate a markdown reportannotations,documents,equations,pic,plots, shared utils/templates) without functional impactWritten by Cursor Bugbot for commit b3f86f8. This will update automatically on new commits. Configure here.