Change manifold to pixel_origin and pixel_deltas

[bendichter]

Motivation

The manifold datatype of ImagingPlane is confusing and inefficient. It is being replaced with pixel_origin and pixel_deltas which specify where the pixels of the image are in space. This assumes a grid layout of the pixels. Non-grids will require an extension.

addresses #554

How to test the behavior?

from pynwb.ophys import OpticalChannel, ImagingPlane
oc = OpticalChannel('test_name', 'test_source', 'description', 500.)
ImagingPlane('test_imaging_plane', 'test source', oc, 'description', 'device', 600.,
                          'imaging_rate', 'indicator', 'location', (.1, .2), (.0, .0), 'reference_frame')

Checklist

• Have you checked our Contributing document?
• Have you ensured the PR description clearly describes problem and the solution?
• Is your contribution compliant with our coding style ? This can be checked running flake8 from the source directory.
• Have you checked to ensure that there aren't other open Pull Requests for the same change?
• Have you included the relevant issue number using #XXX notation where XXX is the issue number ?

[bendichter]

I'm not sure how to fix the last failing test:

======================================================================
FAIL: test_write_cache_spec (test_io.TestHDF5Writer) (neurodata_type='ImagingPlane', test='cached spec preserved original spec')
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/bendichter/dev/pynwb/tests/integration/test_io.py", line 119, in test_write_cache_spec
    self.assertDictEqual(original_spec, cached_spec)
AssertionError: {'att[16 chars]pe': 'text', 'doc': "Value is 'Metadata about [2313 chars]ces'} != {'att[16 chars]pe': u'text', 'doc': u"Value is 'Metadata abou[2366 chars]ces'}
Diff is 8705 characters long. Set self.maxDiff to None to see it.

[codecov]

Codecov Report

Merging #576 into dev will decrease coverage by 0.05%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##              dev     #576      +/-   ##
==========================================
- Coverage   76.83%   76.77%   -0.06%     
==========================================
  Files          59       59              
  Lines        6354     6334      -20     
  Branches     1257     1253       -4     
==========================================
- Hits         4882     4863      -19     
+ Misses       1138     1137       -1     
  Partials      334      334

Impacted Files	Coverage Δ
src/pynwb/ophys.py	83.43% <100%> (-0.21%)	⬇️
src/pynwb/form/utils.py	88.42% <0%> (-0.36%)	⬇️
src/pynwb/base.py	97.87% <0%> (-0.09%)	⬇️
src/pynwb/misc.py	96.05% <0%> (ø)	⬆️
src/pynwb/icephys.py	100% <0%> (ø)	⬆️
src/pynwb/file.py	91.04% <0%> (+0.56%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bde5af9...1057fd5. Read the comment docs.

[ajtritt]

@bendichter just to confirm, this is just a different way of representing the same data, correct?

[bendichter]

well, not exactly. The previous way would allow you to have irregular sampling. This enforces that you must have uniform grids. The irregular sampling will be useful for random sampling techniques, but from what I understand they'll need an extension anyway. The majority of labs use a uniform grid, so this is a much more straightforward and compact way of representing space.

[bendichter]

To me this seems analogous to TimeSeries.timestamps. In that case we provided two options: specify start and delta for constant sampling, or specify all timestamps for non-uniform sampling. Here we could do the same thing, where we allow the user to specify the start and deltas in physical space for cases where each dimension has a constant spatial sampling (this is true most of the time), and we can continue to support the old way, which would work for the rare cases of irregular spatial sampling. This change would allow for a much more compact and intuitive representation of space for most labs.

[rly]

I agree with Ben's last comment here -- allow both methods to work, especially now that we do not want to break backwards compatibility with 2.0.

[rly]

Using pixel_origin and pixel_deltas would still require a unit for the pixel_deltas values. To make pixel spacing like timestamps, I would create a group called uniform_sampling with datasets pixel_origin and pixel_deltas and an attribute unit).

This is different from timestamps (below) where, to be consistent, I would have created a group called uniform_sampling with attributes starting_time, rate, and unit, but it might be too late for that.

For reference, the schema for TimeSeries says:

  - name: starting_time
    dtype: float64
    doc: 'The timestamp of the first sample. COMMENT: When timestamps are uniformly
      spaced, the timestamp of the first sample can be specified and all subsequent
      ones calculated from the sampling rate.'
    attributes:
    - name: rate
      dtype: float32
      doc: 'Sampling rate, in Hz COMMENT: Rate information is stored in Hz'
    - name: unit
      dtype: text
      doc: Value is 'Seconds'
      value: Seconds
    quantity: '?'
  - name: timestamps
    dtype: float64
    doc: 'Timestamps for samples stored in data.COMMENT: Timestamps here have all
      been corrected to the common experiment master-clock. Time is stored as seconds
      and all timestamps are relative to experiment start time.'
    attributes:
    - name: interval
      dtype: int32
      doc: Value is '1'
      value: 1
    - name: unit
      dtype: text
      doc: Value is 'Seconds'
      value: Seconds
    dims:
    - num_times
    quantity: '?'
    shape:
    - null

[bendichter]

I agree about the need for a unit. Let's discuss the options for how to include this information. Making a group seems like it may be more complicated than necessary

[ajtritt]

@bendichter @rly What's the status of this? Tomorrow marks this PR's first birthday, and there hasn't been activity for six weeks.

From my end, this seems reasonable and something that should be included.

@bendichter do you want this to automatically close #554? If so, "addresses" needs to be changed to something from this list

[bendichter]

The status is this racked up merge conflicts but it is still definitely something I want to push through

[bendichter]

I use "addresses" when it's a partial fix and shouldn't close the issue

[rly]

I will resolve the conflicts, set units to be fixed to meters, and make manifold optional and deprecated. How does this sound?

[oruebel]

How does this sound?

Sounds good

[bendichter]

Yeah, sounds good