NeoMatlabIO: round-trip quantity, array and nested annotations

[adityasingh2400]

When NeoMatlabIO writes an object, _get_matlab_value flattens the annotations dict for MATLAB. A quantity is split into a plain magnitude plus a companion <key>_units field, exactly the way quantity attributes such as t_start are stored. A nested mapping becomes a nested struct. None becomes the sentinel string Py_None, because MATLAB has no equivalent and scipy drops a real None.

The read side undid none of that. The elif attrtype == dict branch copied every field of the struct straight into the annotations dict, so a quantity came back as {'yop': array([3., 4., 5.]), 'yop_units': 'ms'} instead of [3, 4, 5] * pq.ms, and a nested mapping came back as a raw scipy.io.matlab.mat_struct object. That is what issue #852 reports.

The same branch has a worse problem that the issue does not mention. It tested the sentinel with if value == PY_NONE, and on an array-valued annotation that comparison returns an array, which is not usable as a condition. So ValueError: The truth value of an array with more than one element is ambiguous came out of read_block for any file holding an array annotation, quantity or not. The file was written without complaint and could then never be read back.

Decoding now mirrors the encoding, in a new create_dict_from_struct alongside the existing create_ob_from_struct. A <key>_units field is folded back into the value it belongs to and is not emitted as a key of its own, but only when the field it names is actually present, so an annotation genuinely called foo_units still round-trips on its own. A nested struct is decoded recursively. The sentinel is tested only on values that are strings. One line changed on the write side too: None inside a nested annotation dict was silently dropped, because the guard that names the annotations attribute did not survive the recursion into the nested mapping, and annotations are the only dict-typed attribute any Neo class declares, which I checked across class_by_name.

Verified with a write then read round trip on a temporary file, which needs no downloaded data. The new parametrized test covers a string, an int, None, a plain array, a quantity array, a quantity scalar, a flat dict and a doubly nested dict holding quantities, and asserts no companion field leaks into the annotations. The annotation the issue asks for, yop=[3, 4, 5] * pq.ms, is also added to test_write_read_single_spike as suggested. Against 35cbce7, neo/test/iotest/test_neomatlabio.py is 6 failed 11 passed before the source change and 17 passed after, and neo/test/coretest stays at 637 passed 0 failed.

One thing I want to flag rather than bury: a quantity and its units are two separate fields in the .mat file, so an annotation dict that genuinely contains both a and a_units is indistinguishable on disk from a single quantity annotation a, and now reads back as the latter. That ambiguity is inherent to the storage format the write side already used, and it is the same convention Neo applies to object attributes, but it is a behaviour change for that one case.

Fixes #852

[apdavison]

This is nice implementation, and well tested. Many thanks for this PR! It just needs a couple of docstring changes for clarity.

For future reference, I should note that I found the PR description too long and difficult to understand, which slowed down the review.

Some general principles for future PRs:

1. Begin by stating the problem it solves. Where there is an existing issue, you can just refer to the issue, and briefly summarise it.
2. Next state how this PR solves the problem. It is not necessary to explain every code change in words, or to give the history of how you solved it, just give a high-level view; anything that needs a detailed commentary should already have code comments to that effect. Use present tense, not past tense, when referring to the state of the code in the master branch (since the PR has not been merged yet).
3. Next explain briefly how the code was tested/validated.
4. Any other comments, such as changes unrelated to the underlying issue.

Applying this to the current PR would give something like this:

---

Problem

Fixes #852.

On master, NeoMatlabIO writes a quantity annotation as a plain magnitude plus a companion <key>_units field, and a nested annotation dict as a nested struct, but the read path reverses neither. An annotation yop=[3, 4, 5] * pq.ms comes back as {'yop': array([3., 4., 5.]), 'yop_units': 'ms'}, and a nested dict comes back as a raw scipy.io.matlab.mat_struct.

While fixing that I hit a second, more serious bug in the same branch of create_ob_from_struct. The None sentinel is tested with if value == PY_NONE, which evaluates to an array when the annotation is array-valued and raises ValueError: The truth value of an array with more than one element is ambiguous. Any object carrying an array annotation is therefore written without complaint and can never be read back.

Solution

Decoding now mirrors encoding, in a new create_dict_from_struct alongside the existing create_ob_from_struct: <key>_units is folded back into the value it belongs to, nested structs are decoded recursively, and the sentinel is tested only on strings.

One change on the write side: None inside a nested annotation dict is currently dropped, because the guard identifying the annotations attribute does not survive the recursion into the nested mapping. Annotations are the only mapping-valued attribute any Neo class declares, so the guard can be dropped.

Testing

neo/test/iotest/test_neomatlabio.py goes from 6 failed / 11 passed to 17 passed, and neo/test/coretest is unaffected. The new parametrized test round-trips a range of value types, and checks that no companion field leaks into the annotations. The yop annotation from #852 is also added to test_write_read_single_spike, as the issue suggests.

Other comments

A quantity and its units are two separate fields in the .mat file, so plain annotations a and a_units are indistinguishable on disk from a single quantity annotation a, and now read back as the latter. The ambiguity is inherent to the storage convention the write side already uses but it is a behaviour change for that case.

[apdavison]

"Undoes" is a bit confusing. Maybe "This reverses that flattening, transforming the string "Py_None" into Python None and the magnitude/units pair into a Quantity scalar or array.

Also, the sentinal string is "Py_None" not "None", isn't it?