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

Require TimeSeries.data and .unit, update ImageSeries, OpticalSeries #1274

Merged
merged 24 commits into from
Aug 10, 2021

Conversation

rly
Copy link
Contributor

@rly rly commented Aug 6, 2020

Motivation

Fix #1220.

This PR includes breaking changes to the API and will thus be pushed out in PyNWB 2.0.

Changes:

  • data is now required in __init__ of TimeSeries and all subclasses of TimeSeries, except for ImageSeries and its subclasses, where external_file can be provided instead of data.

    • This creates a weird situation in the inheritance hierarchy where a subclass does not contain a required field of the superclass. As a result, if data is not passed into the constructor of ImageSeries, then data will be set to the default value so that super().__init__ may be called. This results in the default value being written to disk, which is not ideal. An alternative solution is to split ImageSeries into two separate classes (and neurodata_types), one where data is required and one where external_file is required.
    • If a TimeSeries is read without data, then data will be set to a default value: an empty 1D numpy array. This is also true for all subclasses of TimeSeries except for ImageSeries and its subclasses and DecompositionSeries where the shape of data cannot be 1-D, so the default value is an empty 3D numpy array.
  • unit is now required in __init__ of TimeSeries and all subclasses of TimeSeries, except for ImageSeries and its subclasses.

    • Like for data above, if a TimeSeries is read without unit, then unit will be set to a default value 'unknown'.
    • Like for data above, if an ImageSeries is constructed without unit, then unit will be set to a default value so that super().__init__ may be called. This results in the default value being written to disk, which is not ideal.
    • In ImageSeries, unit is required only if data is provided.
  • Previously, data was required in OpticalSeries.__init__ even though an external_file could be provided. This is a bug, so data is now made optional in OpticalSeries.__init__. This has the side effect of moving the position of data to later in the argument list, which may break code that relies on positional arguments for OpticalSeries.__init__.

  • This is a separate change, but included in this PR. NWB 2.4 introduces TimeSeriesReference. The TimeIntervals table has been updated to use TimeSeriesReference instead of a bare 3-element tuple.

  • Added test files with TimeSeries and ImageSeries types that are missing data and unit.

  • Fixed many tests that used data-less or unit-less TimeSeries objects.

TODO:

  • add more testing
  • update changelog

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?

@rly
Copy link
Contributor Author

rly commented Aug 6, 2020

@oruebel @ajtritt @bendichter Could you please take a look at this PR? It is not ready to merge, but I would like feedback on it before I finalize it. Thanks.

@codecov
Copy link

codecov bot commented Jul 31, 2021

Codecov Report

Merging #1274 (85fed51) into rc/2.0.0 (bbc360b) will decrease coverage by 0.25%.
The diff coverage is 58.33%.

Impacted file tree graph

@@             Coverage Diff              @@
##           rc/2.0.0    #1274      +/-   ##
============================================
- Coverage     77.04%   76.78%   -0.26%     
============================================
  Files            37       37              
  Lines          2692     2731      +39     
  Branches        445      455      +10     
============================================
+ Hits           2074     2097      +23     
- Misses          541      552      +11     
- Partials         77       82       +5     
Impacted Files Coverage Δ
src/pynwb/io/image.py 66.66% <0.00%> (ø)
src/pynwb/io/base.py 55.81% <36.66%> (-6.90%) ⬇️
src/pynwb/base.py 94.68% <100.00%> (+0.05%) ⬆️
src/pynwb/image.py 95.06% <100.00%> (+0.61%) ⬆️
src/pynwb/misc.py 87.40% <100.00%> (+0.09%) ⬆️

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 bbc360b...85fed51. Read the comment docs.

@rly rly changed the title Require TimeSeries.data and .unit except for ImageSeries Require TimeSeries.data and .unit, use TimeSeriesReference Aug 6, 2021
Use TimeSeriesReference in tests

Add test for backwards compatibility of timeintervals

Update ObjectMapper (not yet working)

Fix test

Demonstrate two approaches to resolving mapping issue

Fix
@rly rly marked this pull request as ready for review August 9, 2021 23:47
@rly rly requested a review from oruebel August 9, 2021 23:47
@rly rly changed the title Require TimeSeries.data and .unit, use TimeSeriesReference Require TimeSeries.data and .unit, update ImageSeries, OpticalSeries Aug 10, 2021
@rly rly merged commit dd7e8a3 into rc/2.0.0 Aug 10, 2021
@rly rly deleted the enh/req_ts_data_unit branch August 10, 2021 00:42
rly added a commit that referenced this pull request Aug 10, 2021
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.

TimeSeries can be written without 'data' dataset
2 participants