NeoMatlabIO: round-trip quantity, array and nested annotations

[adityasingh2400]

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. The new parametrized test round-trips a range of value types, and checks that no companion field leaks into the annotations. It writes to a temporary file rather than the downloaded test data, so it also runs without datalad, where the counts are 5 failed / 3 passed to 8 passed with the other 9 skipped. 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]

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?

[adityasingh2400]

Thanks, and you are right on both counts. The sentinel is PY_NONE = "Py_None" at line 62, and my docstring said "stores None as a sentinel string" without naming it. Fixed in 3d5a722, using your wording:

That method flattens a quantity into a plain magnitude plus a companion
``<key>_units`` field, mirrors nested mappings as nested structs, and stores
`None` as the sentinel string ``"Py_None"`` because MATLAB has no equivalent.

This reverses that flattening, transforming the string ``"Py_None"`` into
Python `None`, the magnitude/units pair into a Quantity scalar or array, and
the nested struct back into a dict, so that a value survives a write/read
round trip unchanged.

I also rewrote the PR description to your four-part structure. Noted on length for future PRs.

One correction to my own first draft of that description, in case you read it before I fixed it. I wrote that the tests cover "an array annotation alongside None in the same object", which does not exist. The suite is a single parametrized test with eight cases, str, int, none, array, quantity_array, quantity_scalar, dict and nested_dict. The description now says that.

[adityasingh2400]

Thanks, that is useful and I have rewritten the description to your version.

I checked the numbers before adopting them. Without datalad the file goes 5 failed / 3 passed to 8 passed with the other 9 skipped, which lines up with your 6 / 11 to 17 once the datalad tests run, since test_write_read_single_spike is the sixth failure.

I had also left the write-side guard change out of the description entirely, so that is in now too.

[adityasingh2400]

Thanks for running the CI. The red is the data host rather than this branch, so flagging what it does and does not tell us.

The step runs pytest neo/test/rawiotest and then pytest neo/test/iotest. The first ended at 20 passed, 9 skipped, 96 errors, and all 96 are datalad failing to fetch ephy_testing_data:

fatal: unable to access 'https://gin.g-node.org/NeuralEnsemble/ephy_testing_data/': The requested URL returned error: 403

Every one of the 96 is that same IncompleteResultsError, spread over 30 rawiotest modules, and none is in a file this PR touches. A few timed out instead of refusing, Failed to connect to gin.g-node.org port 443 after 135820 ms.

The job stopped there, so neo/test/iotest never ran. test_neomatlabio.py shows 0 percent in the coverage table for that reason, not because the tests failed. This run says nothing about the change either way.

git ls-remote https://gin.g-node.org/NeuralEnsemble/ephy_testing_data.git succeeds from my machine right now, returning HEAD f5007ec3, so the host is up and is refusing the runners specifically. NeuralEnsemble/elephant is hitting the same thing today on elephant-data.

Locally the file is 8 passed and 9 skipped, where the 9 skips are exactly the datalad cases this run would have covered.