Repository navigation
Conversation
Floating arange endpoints can add an extra timestamp, for example 14 samples at dt=0.01. Construct exactly one time per sample in both model input adaptation and standalone differentiation. Cover public fitting, scoring and three derivative methods against explicit grids and polynomial derivatives.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89d74e644a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ) | ||
| nt = x.shape[-2] | ||
| t = np.arange(0, t * nt, t).reshape((-1, 1)) | ||
| t = (np.arange(nt) * t).reshape((-1, 1)) |
There was a problem hiding this comment.
When a standalone differentiator is called with t=0, the preceding guard only checks t < 0, so this multiplication now constructs an all-zero grid; the replaced np.arange(..., step=t) instead rejected a zero step. For example, SpectralDerivative()(x, t=0) then computes t[1] - t[0] == 0 and returns non-finite derivatives rather than rejecting the invalid timestep as the error text requires, so zero should be rejected before constructing the grid.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Corrected in 49ef461. The standalone guard now rejects t <= 0 before constructing the grid, matching model input validation. Finite and spectral differentiation cover zero and negative spacing; the zero cases fail on the previous PR head and all four cases now pass. The affected suite has 137 passes, and all 18 recorded-waveform results remain unchanged.
Keep the positive-step requirement when constructing times by multiplication. The former arange step rejected zero; now the explicit guard handles it consistently with model input validation. Check finite and spectral methods with zero and negative steps.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
For some sample counts, a valid scalar timestep makes
SINDy.fitfail before fitting. For example, 14 samples att=0.01generate 15 timestamps and raiseValueError: cannot reshape array of size 15 into shape (14,1). Standalone differentiation fails similarly.Build timestamps from integer sample indices multiplied by the timestep in both model input adaptation and
BaseDifferentiation. This gives exactly one time per sample without changing the configured sampling interval. The standalone guard also rejects zero spacing, matching the model input guard. Twelve regressions cover fitting/scoring and finite, smoothed and spline derivatives against explicit grids and analytic polynomial derivatives.Validation:
GurobiError: Unknown parameter max_iter, with current optional dependencies. Baseline: 709 passed, 13 skipped, 11 deselected. Fixed: 725 passed, with the exact same 35 failure IDs, 13 skips and 11 deselections. No optimizer code changes.