Conversation
Ignore relations whose endpoints are absent from the active axes, preventing lookup errors while preserving existing values. Add coverage for missing labels.
Skip the solver when nothing can vary and return a successful result with consistent optimization metadata
To avoid some of the performance regressions. - Avoid slow coordinate comparisons - Use plain coordinate values during label lookup to avoid repeated alignment overhead.
Return a scaled copy so index-dependent matrices keep their original values. Add coverage for preserving the unscaled matrix. This avoids us having to having to mutate the object before plotting
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## staging #1609 +/- ##
=========================================
+ Coverage 87.3% 87.9% +0.6%
=========================================
Files 92 95 +3
Lines 4302 4455 +153
Branches 487 504 +17
=========================================
+ Hits 3758 3920 +162
+ Misses 422 420 -2
+ Partials 122 115 -7 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Greptile SummaryThis PR introduces a native PFID element and broad optimization, activation, data-preparation, compatibility, and metadata corrections.
Confidence Score: 2/5The PR is not yet safe to merge because descending-axis penalties, shifted dispersion metadata, and nonnegative PFID rates can produce materially incorrect fitting or result behavior. Equal-area penalties can disappear on descending axes, activation results can report centers different from those used by the model, and validly constructed PFID rates can silently generate zero basis columns; overlapping intervals and zero-sum activation scales add narrower correctness risks. Files Needing Attention: glotaran/optimization/penalty.py, glotaran/builtin/items/activation/gaussian.py, glotaran/builtin/elements/pfid/matrix.py Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
Config[PFID and activation configuration] --> Matrix[PFID matrix generation]
Matrix --> Simulation[Simulation]
Matrix --> Estimate[Conditional CLP estimation]
Estimate --> Penalties[Equal-area penalties]
Penalties --> Optimizer[Outer optimization]
Optimizer --> Results[Result assembly]
Results --> Decomposition[PFID amplitudes, phases, and concentrations]
Reviews (1): Last reviewed commit: "Avoid mutating shared optimization matri..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| start = int(np.argmin(np.abs(global_axis - lower))) | ||
| stop = int(np.argmin(np.abs(global_axis - upper))) + 1 | ||
| for matrix, estimation in zip(matrices[start:stop], estimations[start:stop], strict=True): |
There was a problem hiding this comment.
On a descending global axis, the interval endpoints produce reversed positional indices. For example, the default interval on [20, 10, 0] yields start=2 and stop=1, so _get_area returns an empty area and every equal-area penalty silently becomes zero. Select points by their coordinate values rather than assuming ascending positional order.
Knowledge Base Used: Optimization workflow
| for interval_lower, interval_upper in intervals: | ||
| if interval_lower > global_axis[-1]: | ||
| continue | ||
| lower = max(interval_lower, np.min(global_axis)) | ||
| upper = min(interval_upper, np.max(global_axis)) | ||
| if lower > upper: | ||
| lower, upper = upper, lower | ||
| start = int(np.argmin(np.abs(global_axis - lower))) | ||
| stop = int(np.argmin(np.abs(global_axis - upper))) + 1 | ||
| for matrix, estimation in zip(matrices[start:stop], estimations[start:stop], strict=True): | ||
| if label in matrix.clp_axis: | ||
| area.append(estimation.clp[matrix.clp_axis.index(label)]) |
There was a problem hiding this comment.
Values are appended independently for every interval, so a coordinate is counted multiple times when intervals overlap. EqualAreaPenalty accepts overlapping intervals, while its existing applies() behavior uses any(...) and counts each coordinate once. This inflates the calculated area and changes the optimization objective for valid configurations.
Knowledge Base Used: Optimization workflow
| axis_values = np.asarray(axis, dtype=np.float64) | ||
| dispersion_center = float(self.dispersion_center) | ||
| distance = ( | ||
| (1e3 / axis_values - 1e3 / dispersion_center) | ||
| if self.reciproke_global_axis | ||
| else (axis_values - dispersion_center) / 100 | ||
| ) | ||
| center_columns = center[:, np.newaxis] * np.ones((nr_gaussians, axis_values.size)) | ||
| for i, coefficient in enumerate(self.center_dispersion_coefficients): | ||
| center_columns += float(coefficient) * np.power(distance, i + 1) |
There was a problem hiding this comment.
When an activation has both wavelength-dependent dispersion and explicit per-index shifts, this calculation starts from the base centers and applies only the dispersion coefficients. Matrix generation applies each shift before dispersion, so result datasets report centers that differ from those actually used during fitting.
Knowledge Base Used: Optimization workflow
| shifted_axis = model_axis - center | ||
| left_shifted_axis_indices = np.where(shifted_axis < 5 * width)[0] | ||
| left_shifted_axis = shifted_axis[left_shifted_axis_indices] | ||
| negative_rate_indices = np.where(rates < 0)[0] |
There was a problem hiding this comment.
PFID rates are unconstrained, but this matrix calculation populates only entries whose rate is negative. A configured nonnegative rate, or an unconstrained fit trial that crosses zero, therefore produces all-zero cosine and sine basis columns without an error. The affected oscillation silently disappears from simulation and fitting.
Knowledge Base Used:
| parameter.scale, | ||
| global_axis[index], | ||
| ) | ||
| matrix[index] /= sum(parameter.scale for parameter in parameters) |
There was a problem hiding this comment.
The sum of Gaussian scales is used as a divisor without validation, even though configured scales can be zero or cancel each other. A zero normalization sum propagates NaNs through the PFID matrix and into simulation or optimization. Reject this configuration with a clear error before dividing.
Knowledge Base Used:
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved PFID rate, Gaussian shift, penalty interval, overlap, and parameter-rendering issues remain.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Ports native PFID support to staging and adds optimization, compatibility, and numerical robustness improvements.
Changes:
- Adds PFID modeling, simulation, optimization, registration, and tests.
- Improves penalties, parameter handling, matrix ordering, and zero-parameter optimization.
- Updates compatibility, kinetic normalization, SVD preparation, ASCII output, changelog, and linting.
File summaries
| File | Summary |
|---|---|
tests/simulation/test_simulation.py |
Tests dimension-independent seeded noise. |
tests/parameter/test_parameters.py |
Tests missing-expression handling. |
tests/parameter/test_parameters_rendering.py |
Tests near-zero error rendering. |
tests/parameter/test_parameter.py |
Tests expression normalization. |
tests/optimization/test_relations_and_constraints.py |
Tests unresolved relations. |
tests/optimization/test_optimization.py |
Tests optimization without used free parameters. |
tests/optimization/test_objective.py |
Tests equal-area penalties. |
tests/optimization/test_matrix.py |
Tests matrix ordering and scaling. |
tests/io/test_prepare_dataset.py |
Tests dimension-aware SVD preparation. |
tests/builtin/io/ascii/test_explicit_file_reader.py |
Tests ASCII formats. |
tests/builtin/elements/pfid/test_pfid_element.py |
Tests PFID functionality. |
tests/builtin/elements/pfid/__init__.py |
Initializes PFID tests. |
tests/builtin/elements/kinetic/test_kinetic_element.py |
Tests activation normalization. |
requirements_pinned.txt |
Adds pandas version markers. |
pyproject.toml |
Registers the PFID plugin. |
glotaran/simulation/simulation.py |
Canonicalizes seeded noise dimensions. |
glotaran/parameter/parameters.py |
Handles near-zero standard errors. |
glotaran/parameter/parameter.py |
Normalizes expression values. |
glotaran/optimization/penalty.py |
Reworks equal-area penalty calculation. |
glotaran/optimization/optimization.py |
Supports no-free-parameter runs. |
glotaran/optimization/objective.py |
Preserves penalty metadata and improves amplitude gathering. |
glotaran/optimization/matrix.py |
Preserves CLP order and avoids scale mutation. |
glotaran/optimization/estimation.py |
Skips invalid CLP relations. |
glotaran/io/prepare_dataset.py |
Makes SVD dimension-aware. |
glotaran/builtin/items/activation/gaussian.py |
Updates Gaussian dispersion calculation. |
glotaran/builtin/io/ascii/wavelength_time_explicit_file.py |
Improves NumPy coordinate output. |
glotaran/builtin/elements/pfid/matrix.py |
Implements PFID matrix generation. |
glotaran/builtin/elements/pfid/element.py |
Implements PFID model behavior and results. |
glotaran/builtin/elements/pfid/__init__.py |
Exports PFID classes. |
glotaran/builtin/elements/kinetic/element.py |
Fixes activation normalization. |
changelog.md |
Documents the changes. |
.pre-commit-config.yaml |
Adjusts Taplo linting. |
Review details
Suppressed comments (2)
glotaran/optimization/penalty.py:36
- Overlapping intervals are accumulated independently here, so a global-axis point covered by two intervals contributes twice. This differs from
EqualAreaPenalty.applies, which treats the interval list as anany/union; for example,[(0, 2), (1, 3)]should not double-count the shared points.
for matrix, estimation in zip(matrices[start:stop], estimations[start:stop], strict=True):
if label in matrix.clp_axis:
area.append(estimation.clp[matrix.clp_axis.index(label)])
glotaran/parameter/parameters.py:562
- The new threshold is applied only to the table renderer here.
Parameter.markdown()still divides bystandard_errorwhenever it is not NaN, so a zero/near-zero error can still raise or render an infinite t-value when parameters are rendered directly or through nested expressions. Reuse the same threshold in that renderer as well.
standard_error = (
np.nan
if abs(parameter.standard_error) < MINIMUM_STANDARD_ERROR
else parameter.standard_error
)
- Files reviewed: 32/32 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| negative_rate_indices = np.where(rates < 0)[0] | ||
|
|
| if self.dispersion_center is None: | ||
| return np.array([np.full(axis.size, c) for c in center]) |
| if interval_lower > global_axis[-1]: | ||
| continue |
Always persist scale and weighted error metadata, using the unweighted fit when no weights are present. Pin semantic validation to the reviewed reference and retain its report for troubleshooting.
Advance the dependency revision to bring the latest feature set into staging.
pyyaml isn't a dependency of us and thus not guarantee to be installed. See CI failure: https://github.com/glotaran/pyglotaran/actions/runs/34787513732/job/103805630377?pr=1609 In a full dev env it is installed because of doc and validation dependencies. Run: uv tree | rg pyyaml --passthru
|
Changes here have been validated on @jsnel 's
0_8-pygta-validation-workspaceand discussed on Slack.Change summary
Checklist
Closes issues
closes #XXXX