Fix problem with dimensionality - #194
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request fixes the behavior of single-dimension slicing in the Parser class to preserve the dimension axis. Previously, selecting a single dimension (e.g., 'x', 'y', or 'z') would eliminate the dimension axis entirely, while multi-dimension selections would preserve it. The fix ensures consistent behavior across all dimension selections.
- Changed single-dimension slice definitions from index-based (
np.s_[0]) to range-based (np.s_[0:1]) to preserve the dimension axis with size 1 - Added three new test cases to verify the dimension axis size for single, double, and triple dimension selections
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| kinisi/parser.py | Updated DIMENSIONALITY dictionary to use slice ranges instead of single indices for 'x', 'y', 'z' dimensions |
| kinisi/tests/test_parser.py | Added test cases to verify that dimension axis is preserved with correct sizes (1, 2, 3) for different dimension selections |
Comments suppressed due to low confidence (1)
kinisi/parser.py:31
- The byte string versions of single dimensions ('b'x'', 'b'y'', 'b'z'') still use single-index slicing (np.s_[0], np.s_[1], np.s_[2]) instead of range-based slicing. This is inconsistent with the updated string versions and will cause the same dimension-elimination bug if byte string dimensions are used (e.g., from HDF5 loading). These should be updated to 'np.s_[0:1]', 'np.s_[1:2]', and 'np.s_[2:3]' respectively to match the fix applied to the string versions.
b'x': np.s_[0],
b'y': np.s_[1],
b'z': np.s_[2],
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Removed @PythonFZ, cause I feel like he is doing a lot of reviewing for us 🤣. |
|
Seems like a fairly small change and covered by new tests so I am happy. |
Harry-Rich
left a comment
Harry-Rich
left a comment
There was a problem hiding this comment.
Small change and covered by tests
As highlighted with #193, currently inputting a single dimension doesn't work. This PR resolves this issue and adds appropriate tests.