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

[Bug]: Syntax valid only after Matlab 2018b #437

Closed
2 tasks done
ftadel opened this issue Jun 22, 2022 · 4 comments · Fixed by #438
Closed
2 tasks done

[Bug]: Syntax valid only after Matlab 2018b #437

ftadel opened this issue Jun 22, 2022 · 4 comments · Fixed by #438

Comments

@ftadel
Copy link

ftadel commented Jun 22, 2022

What happened?

The syntax maxVal = max(val, [], 'all'); is used here:
https://github.com/NeurodataWithoutBorders/matnwb/blob/master/%2Btypes/%2Butil/correctType.m#L34

It appeared in Matlab 2018b and crashes before that.
But it seems like matnwb should be working with older versions of Matlab.

This newer syntax can be easily replaced with:
maxVal = max(val(:));

This error was reported on the Brainstorm forum:
https://neuroimage.usc.edu/forums/t/error-opening-nwb-files/21025/20

Steps to Reproduce

Using Matlab 2017b

Error Message

** Error: Line 34: max
** Invalid option. Option must be 'omitnan' or 'includenan'.
**
** Call stack:
** >correctType.m at 34
** >checkDtype.m at 111
** >DynamicTableRegion.m>DynamicTableRegion.validate_data at 34
** >Data.m>Data.set.data at 29
** >Data.m>Data.Data at 22
** >VectorData.m>VectorData.VectorData at 13
** >DynamicTableRegion.m>DynamicTableRegion.DynamicTableRegion at 13
** >parseDataset.m at 72
** >parseGroup.m at 22
** >parseGroup.m at 38
** >parseGroup.m at 38
** >nwbRead.m at 59

Operating System

Windows

Matlab Version

2017b

Code of Conduct

@bendichter
Copy link
Contributor

Thanks for raising this @ftadel. It's good to have documentation of this issue in case others run into it. Thanks for the fix suggestion. I think in this particular case we might be able to just use the more generic syntax you suggest for compatibility.

This does raise a bigger question though of what versions of MATLAB we want to support in cases where the changes would need to be more substantial. I think going more than 5 years back might be difficult. Certainly certain features such as dynamically loaded filters would not be available. I was under the impression that most MATLAB users have a license that entitles them to download the latest version, so I am surprised to see such an old version being actively used. @lawrence-mbf, what do you think? Do you have a sense of what versions of MATLAB our users are using and how difficult it would be to support the tail of that distribution?

@lawrence-mbf
Copy link
Collaborator

For ScanImage right now we just bumped it up to 2018a. My assumption was the same as yours that most universities have a site-wide license which allows them access to the newest release.

@ftadel
Copy link
Author

ftadel commented Jun 22, 2022

My assumption was the same as yours that most universities have a site-wide license which allows them access to the newest release.

Unfortunately, this is true in rich universities of rich countries only.
On the Brainstorm forum, we commonly address requests from users with Matlab license older than 5yrs.

When it does not require much extra work (like in the example above with max), we try to always favor the most backward-compatible syntax.
Similarly, for all the functions from specialized commercial toolboxes, we try to provide alternatives using only the basic install of Matlab (e.g. hilbert.m from the Signal Processing toolbox replaced with an alternative adapted from Octave).

@lawrence-mbf
Copy link
Collaborator

That's good to know. I guess for now we'll try and limit ourselves to 2017a and if there is a need to go older we can address that specifically.

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 a pull request may close this issue.

3 participants