Skip to content

Fix conversion of tables with char array columns - #63

Open
h-mayorquin wants to merge 1 commit into
foreverallama:mainfrom
h-mayorquin:support_string_partition
Open

h-mayorquin wants to merge 1 commit into
foreverallama:mainfrom
h-mayorquin:support_string_partition

Conversation

@h-mayorquin

Copy link
Copy Markdown

Hi, I maintain neuroconv, a library used to convert neurophysiology data to a standard called NWB. We use your library among others to read a lot of MATLAB files because that's a format in which labs commonly write, so first, big thanks.

Now, I have found an issue. When reading some lab tables with load_from_mat I realized that a table with a char array column like table(['ab ';'cde']) is not converted and comes back as MatlabOpaque with a "tuple index out of range" warning. decode_char_arrays removes the char axis so the column reaches to_dataframe as a one-dimensional array with one string per row, and coldata.shape[1] fails.

This only happens for tables saved with R2024b or older (versionSavedFrom 4 or lower), as tables saved with R2025a or newer (versionSavedFrom 5) are not supported yet.

This PR fixes this. For testing, I generated new files for the test data (test_table_char_v7.mat and test_table_char_v73.mat) with their own script (note that I used R2024b). If you want me to modify one of your files instead I am ok with it.

The calendarDuration tests fail with numpy 2.5 or newer, with and without this change, and I will open a separate issue for that soon.

@foreverallama

Copy link
Copy Markdown
Owner

Glad to know that the library is helpful! The PR looks great! I'll merge this after the fix to calendarDuration since the tests will fail otherwise.

I'll try to support MATLAB table v5 as soon as I can

@foreverallama

Copy link
Copy Markdown
Owner

I'm a bit rusty with this. Is it possible to add a save roundtrip test as well, or is that duplicated through a different test?

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants