Avoid raising data timestamps length mismatch warning with external file

[weiglszonja]

Motivation

Minor issue, but storing images (e.g. ImageSeries) with external mode raises a UserWarning that the length of data does not match the length of timestamps. However when external_file is specified (not None), data is not used and therefore this kind of warning should not be raised.

pynwb/src/pynwb/base.py

Lines 184 to 185 in d38cc78

	warn("Length of data does not match length of timestamps. Your data may be transposed. Time should be on "
	"the 0th dimension")

How to test the behavior?

New unittest is added at test_image.ImageSeriesConstructor.test_external_file_no_warning to test new behavior.

Checklist

• Did you update CHANGELOG.md with your changes?
• Have you checked our Contributing document?
• Have you ensured the PR clearly describes the 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 "Fix #XXX" notation where XXX is the issue number? By including "Fix #XXX" you allow GitHub to close issue #XXX when the PR is merged.

[codecov]

Codecov Report

Merging #1486 (a112620) into dev (cf0ee6d) will increase coverage by 0.12%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              dev    #1486      +/-   ##
==========================================
+ Coverage   78.30%   78.42%   +0.12%     
==========================================
  Files          37       37              
  Lines        2761     2777      +16     
  Branches      488      493       +5     
==========================================
+ Hits         2162     2178      +16     
  Misses        518      518              
  Partials       81       81              

Impacted Files	Coverage Δ
src/pynwb/base.py	100.00% <100.00%> (ø)
src/pynwb/image.py	87.37% <100.00%> (+1.06%)	⬆️

---

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 cf0ee6d...a112620. Read the comment docs.

[bendichter]

from @oruebel: better would be to implement the check as a protected method in TimeSeries and overwrite that method in ImageSeries

[weiglszonja]

@rly I have an issue with this, since this check is in the TimeSeries init, I'm not sure how
to pass the external_file or format as kwargs. I noticed this TODO,

pynwb/src/pynwb/image.py

Line 76 in a62c4e9

# TODO catch warning when default data is used and timestamps are provided

which is connected to this warning I believe. So instead of overwriting that method with an additional check, I could ignore the warning when external file is provided within the ImageSeries init?

[bendichter]

does this method need to be static?

[weiglszonja]

No, but currently that would not help I think, because the check method gets called here

pynwb/src/pynwb/image.py

Line 77 in a62c4e9

super().__init__(**kwargs)

before setting self.external_file with whatever value is in args_to_set

pynwb/src/pynwb/image.py

Lines 78 to 82 in a62c4e9

	if args_to_set["external_file"] is None:
	args_to_set["starting_frame"] = None # overwrite starting_frame
	for key, val in args_to_set.items():
	setattr(self, key, val)

[weiglszonja]

Thank you @rly!

[bendichter]

@weiglszonja add to changelog please :-)

[bendichter]

Can we also add a test to ensure that using an external file and a rate instead of timestamps does not raise a warning?

[weiglszonja]

Sure, I added a new unittest.