fix: handle 1D interpolation result in resample for single-state ODEs#75
Closed
yasumorishima wants to merge 4 commits intonasa:mainfrom
Closed
fix: handle 1D interpolation result in resample for single-state ODEs#75yasumorishima wants to merge 4 commits intonasa:mainfrom
yasumorishima wants to merge 4 commits intonasa:mainfrom
Conversation
When an ODESystem has only a single state variable, scipy interpolation returns a 1D array (n,) instead of 2D (n, 1). This causes a broadcast error when assigning to the pre-allocated 2D xs array. Fix by checking ndim and adding a column axis when needed. Fixes nasa#70
6 tests covering: - Basic single-state resample - Resampled values match analytical solution (exponential decay) - include_output=False path - dynamic_output with single-state ODE - Different dt values - Multi-state ODE regression (harmonic oscillator) Also discovered that include_events=True triggers a separate IndexError (issue nasa#71, t_size calculation off-by-2) — tests use include_events=False to isolate the ndim fix from that pre-existing bug. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Address CodeRabbit review: assert resampled.velocity values match analytical solution (-0.5 * exp(-0.5 * t)), not just successful execution. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Thanks for submitting - I ended up addressing this along with a bunch of other issues in #73. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix
TrajectoryAnalysis.resamplefailing forODESystems with a single state variable.When the system has only one state, scipy's interpolation returns a 1D array
(n,)instead of 2D(n, 1), causing a broadcast error on assignment to the pre-allocated 2Dxsarray.The fix checks
ndimand adds a column axis when needed.Fixes #70
Test plan
TestResampleSingleStateclass:include_output=Falsepathdynamic_outputwith single-state ODEdtvaluesNote
During testing, discovered that
include_events=True(the default) triggers a separateIndexErrorat line 741 due tot_sizeunder-allocation. This is the same bug reported in #71 and is not addressed in this PR. All tests useinclude_events=Falseto isolate the ndim fix.