Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix mat support #1337

Merged
merged 4 commits into from
Apr 26, 2024
Merged

fix mat support #1337

merged 4 commits into from
Apr 26, 2024

Conversation

pgunn
Copy link
Member

@pgunn pgunn commented Apr 26, 2024

This fixes use of scipy APIs that are no longer in scipy to read old-style .mat files. ( #1336 )

Support for the newer hdf5-based mat format is retained (and by newer we're talking about a change made a long time ago).

There were two other ways to fix the import problem:

  1. Have a slightly friendlier user message for people trying to load old mat files, by doing try/except in a few places, catching the exception, and guessing that they were trying to load the earlier format and tell them.
  2. Attempt to use scipy.io.matlab.loadmat() instead, building support for that back into the codebase (classic mat handling had been broken for awhile and would've needed some work)

I decided not to do the first because it complicates some already complicated code for the sake of an obsolete format (and the try/exception model actually wouldn't tell us for sure why an OSError was thrown so it could be confusing no matter what).
I decided not to do the second because it's a bit of work for a format people shouldn't be using anymore and it's something we may have decided to stop supporting anyhow even if the code was working (unless users told us they need it).

If some users of old-style mat files turn up and tell us they really need the old format (even though it hasn't worked in caiman for several releases at least), I can move to the second solution, but I'm otherwise happy with this choice.

@pgunn
Copy link
Member Author

pgunn commented Apr 26, 2024

Ugh, I based this off the wrong branch. Redoing the diff.

@ethanbb
Copy link
Contributor

ethanbb commented Apr 26, 2024

I would mildly favor at least catching whatever error gets thrown for older .mat files and erroring with a message that suggests that as a possible cause of the error if the extension is .mat, given how like I said the older format is still the default. But I do support dropping any appearance of real support for it.

@pgunn
Copy link
Member Author

pgunn commented Apr 26, 2024

Hm. Okay. The code for that isn't too bad/bulky.

@pgunn pgunn merged commit df5b487 into dev Apr 26, 2024
3 checks passed
@pgunn pgunn deleted the dev-fix_mat_support branch April 26, 2024 21:33
@ethanbb
Copy link
Contributor

ethanbb commented Apr 26, 2024

Thanks! Yeah, the days of intentionally saving non-HDF5 mat files are long gone (for everyone I know), but the days of accidentally ending up with them are unfortunately still with us.

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