Expand interp cart grid testing#574
Conversation
|
Related to #572 |
waltsims
force-pushed
the
expand_interp_cart_grid_testing
branch
from
e51338a to
290c1d3
Compare
waltsims
force-pushed
the
expand_interp_cart_grid_testing
branch
from
1cedec5 to
8974a47
Compare
This comment was marked as outdated.
This comment was marked as outdated.
|
I have no idea why ruff/isort is not working, although it doesn't matter too much. Have have increased the test coverage for interp_cart_data, |
|
ok, i have absolutely no idea why ruff/isort fails and although it is not important, i would like to know why. As for the test coverage - I am not sure where the motifications of |
|
@greptile-app |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| def test_interp_cart_data_2_points_linear(): | ||
| kgrid = kWaveGrid([1000, 100, 10], [1, 1, 1]) | ||
| binary_sensor_mask = np.zeros((1000, 100, 10), dtype=bool) | ||
| binary_sensor_mask[501, 51, 7] = True | ||
| cart_sensor_mask = np.array([[0.0, 0.0, 0.0], [0.0, 0.0, 2.0]], dtype=np.float32).T # sensor at the origin and another point | ||
| cart_sensor_data = np.array([[1.0, 2.0, 3.0], [4.0, 5.0, 6.0]], dtype=np.float32) # 3 time steps | ||
| logging.debug(cart_sensor_data) | ||
| interp_data = interp_cart_data(kgrid, cart_sensor_data, cart_sensor_mask, binary_sensor_mask, "linear") | ||
| # TODO: find expected value from matlab. In this case we revert to nearest because point is not between p1 and p2. | ||
| logging.debug(interp_data) | ||
| # assert np.allclose(interp_data, cart_sensor_data), "not close enough" |
There was a problem hiding this comment.
test_interp_cart_data_2_points_linear calls interp_cart_data with interp="linear" but the only assertion (assert np.allclose(...)) is commented out with a TODO. The test runs without verifying any result — it will never fail even if the linear interpolation is completely wrong. Since the stated goal of this PR is to expand testing, this should either be completed with a concrete expected value or the test should be marked @pytest.mark.skip with an explanation so it is clearly not a passing validation.
| execution_options = SimulationExecutionOptions( | ||
| is_gpu_simulation=True, | ||
| delete_data=False, | ||
| verbose_level=2) |
There was a problem hiding this comment.
is_gpu_simulation=True in the standalone Python script but the companion notebook sets it to False. Running this file on any machine without a CUDA GPU will fail at runtime. The notebook's default is the safer choice for a portable example.
| execution_options = SimulationExecutionOptions( | |
| is_gpu_simulation=True, | |
| delete_data=False, | |
| verbose_level=2) | |
| execution_options = SimulationExecutionOptions( | |
| is_gpu_simulation=False, | |
| delete_data=False, | |
| verbose_level=2) |
| cart_sensor_data0 = np.array([[1.0, 2.0, 3.0], [4.0, 5.0, 6.0]], dtype=np.float32) # 3 time steps, shape: (2,3) | ||
| print(cart_sensor_data0) | ||
| interp_data0 = interp_cart_data(kgrid, cart_sensor_data0, cart_sensor_mask, binary_sensor_mask) | ||
| print(interp_data0) | ||
| # TODO: find expected value from matlab, current behavior is round up to nearest neighbor | ||
| assert np.allclose(interp_data0, cart_sensor_data0), "not close enough" | ||
| cart_sensor_data1 = np.array([[1.0, 2.0, 3.0, 4.0, 5.0], [6.0, 7.0, 8.0, 9.0, 10.0]], dtype=np.float32) # 5 time steps, shape: (2,5) | ||
| print(cart_sensor_data1) | ||
| interp_data1 = interp_cart_data(kgrid, cart_sensor_data1, cart_sensor_mask, binary_sensor_mask) | ||
| print(interp_data1) |
There was a problem hiding this comment.
print() calls in test code pollute captured output and make pytest -s noisy. Use logging.debug() to keep output clean.
| cart_sensor_data0 = np.array([[1.0, 2.0, 3.0], [4.0, 5.0, 6.0]], dtype=np.float32) # 3 time steps, shape: (2,3) | |
| print(cart_sensor_data0) | |
| interp_data0 = interp_cart_data(kgrid, cart_sensor_data0, cart_sensor_mask, binary_sensor_mask) | |
| print(interp_data0) | |
| # TODO: find expected value from matlab, current behavior is round up to nearest neighbor | |
| assert np.allclose(interp_data0, cart_sensor_data0), "not close enough" | |
| cart_sensor_data1 = np.array([[1.0, 2.0, 3.0, 4.0, 5.0], [6.0, 7.0, 8.0, 9.0, 10.0]], dtype=np.float32) # 5 time steps, shape: (2,5) | |
| print(cart_sensor_data1) | |
| interp_data1 = interp_cart_data(kgrid, cart_sensor_data1, cart_sensor_mask, binary_sensor_mask) | |
| print(interp_data1) | |
| cart_sensor_data0 = np.array([[1.0, 2.0, 3.0], [4.0, 5.0, 6.0]], dtype=np.float32) # 3 time steps, shape: (2,3) | |
| logging.debug(cart_sensor_data0) | |
| interp_data0 = interp_cart_data(kgrid, cart_sensor_data0, cart_sensor_mask, binary_sensor_mask) | |
| logging.debug(interp_data0) | |
| # TODO: find expected value from matlab, current behavior is round up to nearest neighbor | |
| assert np.allclose(interp_data0, cart_sensor_data0), "not close enough" | |
| cart_sensor_data1 = np.array([[1.0, 2.0, 3.0, 4.0, 5.0], [6.0, 7.0, 8.0, 9.0, 10.0]], dtype=np.float32) # 5 time steps, shape: (2,5) | |
| logging.debug(cart_sensor_data1) | |
| interp_data1 = interp_cart_data(kgrid, cart_sensor_data1, cart_sensor_mask, binary_sensor_mask) | |
| logging.debug(interp_data1) |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| if 0 <= c1 <= 1: | ||
| # Linear interpolation | ||
| binary_sensor_data[point_index, :] = (1 - c1) * cart_sensor_data[indices[0], :] + c1 * cart_sensor_data[indices[1], :] | ||
| else: | ||
| # Point is not between p1 and p2 | ||
| # Option 1: Use nearest neighbor | ||
| binary_sensor_data[point_index, :] = cart_sensor_data[indices[0], :] |
There was a problem hiding this comment.
When the target binary-sensor point projects outside the segment between the two nearest Cartesian points (c1 < 0 or c1 > 1), the code silently uses nearest-neighbour without any log message. A caller who explicitly requested interp="linear" will receive nearest-neighbour results with no indication. At minimum, logging.debug (or logging.warning) should be emitted so users can diagnose unexpected interpolation behaviour.
This extends the tests for ìnterp_cart_grid` so that different sensor data is stored at different positions.
Greptile Summary
This PR expands test coverage for
interp_cart_dataandgrid2cartby introducing phase-varied sensor data (so each sensor position carries a distinct signal), fixing the MATLAB 2D reference collector to usemakeCartCircleinstead of the wrongmakeCartSphere, rewriting the brokenlinearinterpolation branch with a proper projection-based approach, and adding a new 2D circular-sensor time-reversal example.kwave/utils/conversion.py— fixed the always-false0 <= dim > 3dimension guard and added MATLAB-compatible Fortran-order sorting togrid2cart, aligningcart_dataandorder_indexwith MATLAB's output.kwave/utils/interp.py— replaced the completely brokenlinearpath (which callednp.appendnon-in-place) with a correct projection-based two-point interpolation and added@typecheckerwith jaxtyping annotations.linearinterpolation test is present but its assertion is commented out.Confidence Score: 3/5
The core library changes look correct and the CI infrastructure regenerates MATLAB reference data automatically, but two concrete gaps remain before merging.
The
test_interp_cart_data_2_points_lineartest runs but asserts nothing — the key assertion is commented out — so the newly rewritten linear interpolation path has no automated correctness check. Separately,pr_2D_circular_sensor.pyhardcodesis_gpu_simulation=Truewhile its companion notebook correctly usesFalse, meaning the example script will crash on any CPU-only machine.tests/test_utils.py(commented-out assertion in the linear test) andexamples/pr_2D_circular_sensor/pr_2D_circular_sensor.py(is_gpu_simulation=True).Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A["interp_cart_data(kgrid, cart_sensor_data,\ncart_sensor_mask, binary_sensor_mask, interp)"] --> B{dim in 2 or 3?} B -- No --> ERR1["ValueError: must be 2D or 3D"] B -- Yes --> C{cart mask shape matches kgrid.dim?} C -- No --> ERR2["ValueError: dimension mismatch"] C -- Yes --> D{binary mask ndim matches kgrid.dim?} D -- No --> ERR3["ValueError: binary mask dim mismatch"] D -- Yes --> E["grid2cart - cart_bsm now Fortran-order sorted"] E --> F["Loop over binary sensor points"] F --> G{interp mode} G -- nearest --> H["argmin dist - copy cart row"] G -- linear --> I{n_cart_points ge 2?} I -- No --> ERR4["ValueError: not enough points"] I -- Yes --> J["sort by dist, get p1 and p2"] J --> K{project target onto p1 to p2 segment} K -- 0 le c1 le 1 --> L["Linear blend result"] K -- outside segment --> M["Nearest-neighbor fallback silent"] G -- other --> ERR5["ValueError: unknown interp"] H --> OUT["binary_sensor_data"] L --> OUT M --> OUTReviews (1): Last reviewed commit: "Merge branch 'master' into expand_interp..." | Re-trigger Greptile