Enh/ ImageSeries: add checks when external_file is used

[weiglszonja]

Motivation

This PR is a continuation of the issue described in #1510 by raising a ValueError when starting_frame does not have the same length as external_file.

• Added a method to NWBMixin that only raises an error when a check is violated on instance creation,
  otherwise throws a warning when reading from a file.

The new checks is used with this method to ensure that that files with invalid data can be read, but prohibits the user from creating new instances when these checks are violated.

New checks:

• The length of starting_frame must match the length of external_file when external_file is specified
• Format must be 'external' when external_file is specified
• data must be empty array when external_file is specified
  ValueError is raised when a check is violated on instance creation,
  but only warns when file is read from disk.

Default values:

• starting_frame is [0] when external_file has a length of 1
• format is 'external' when format was originally None and external_file is specified (DeprecationWarning is raised)

Fix #1510

How to test the behavior?

Unittests are added in tests/unit/test_image.py

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 #1516 (f6c14fb) into dev (2de33c0) will increase coverage by 0.12%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##              dev    #1516      +/-   ##
==========================================
+ Coverage   90.52%   90.65%   +0.12%     
==========================================
  Files          25       25              
  Lines        2460     2494      +34     
  Branches      456      466      +10     
==========================================
+ Hits         2227     2261      +34     
  Misses        148      148              
  Partials       85       85              

Flag	Coverage Δ
integration	69.60% <54.05%> (-0.28%)	⬇️
unit	84.16% <100.00%> (+0.21%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/pynwb/core.py	76.13% <100.00%> (+1.74%)	⬆️
src/pynwb/image.py	90.07% <100.00%> (+2.69%)	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[weiglszonja]

There are tests in tests/unit/test_ophys.py that are affected by this change,
for instance the ImageSeries used in many tests (actually in integration tests as well) define a single external_file but the corresponding starting_frame has a different length:

pynwb/tests/unit/test_ophys.py

Lines 46 to 55 in b11bffc

	iSS = ImageSeries(
	name='test_iS',
	data=np.ones((2, 2, 2)),
	unit='unit',
	external_file=['external_file'],
	starting_frame=[1, 2, 3],
	format='tiff',
	timestamps=[1., 2.]
	)

@oruebel, should I change all of these or what is the best way to proceed?

[oruebel]

should I change all of these or what is the best way to proceed?

Yes, I think updating the tests is the right approach. Looking at those tests, I believe the starting_frame is indeed set incorrectly in those tests.

[weiglszonja]

@oruebel Seems like back compatibility tests are failing, how can I resolve them?

[oruebel]

@oruebel Seems like back compatibility tests are failing, how can I resolve them?

I'll need to have a look at the tests. @rly thoughts?

[oruebel]

It would be nice to have also a test for Line 136 to check that warning are raised on read. Creating invalid files to test all these options may be too involved just to test this behavior, but I think we could just copy the pattern from the ObjectMapper itself in the test:

https://github.com/hdmf-dev/hdmf/blob/7be40229d778dea940134753de957f2457a11e1f/src/hdmf/build/objectmapper.py#L1257-L1262

I.e., call __new__ explicitly to set in_construct_mode=True and then call __init__ with the invalid data to check if the warnings are raised as expected. I.e., a test would look something like:

def test_warn_on_bad_starting_frame():
     obj = ImageSeries.__new__(ImageSeries, container_source=None, parent=None, object_id="test", in_construct_mode=True)
      with self.assertWarnsWith(ValueError,  "...message..."):
           obj.__init__(........arguments for ImageSeries.....)
     obj._in_construct_mode = False 

Setting obj._in_construct_mode = False should only be necessary of you continue to the use the object.

[oruebel]

Ok, looking at the tests, it looks like you are already testing this with actual files. Maybe the backward compatibility tests are not counted in codecov? @rly

@weiglszonja so I don't think we need to add more tests, but it would still be interesting to know why codecov indicates that Line 136 ids not covered when it seems like it should be based on the tests you added.

[rly]

Backward compatibility tests are currently not counted in codecov, but now that we are including integration tests in codecov, I think it makes sense to include the backward compatibility tests too.

[oruebel]

Thanks @rly for the clarification. That makes sense. I added a couple of extra unit-tests anyways to cover this case there as well. I think those tests are useful also because they show how we can write unittests that mimic the use-case were we need to distinguish between construct mode on read and regular user mode.

[oruebel]

@weiglszonja overall this PR looks good. Normally, I would have just approved this PR as is, however, since this is the first example where we distinguish between warning on read and error on new, I think it will be useful to spent the extra effort to have the pattern we use for this in the code as clean as possible as others will likely follow the pattern we set here in the future. I think the suggestions I made should only require some minor reshuffling of lines of code, so hopefully the extra effort should not be too much. Thanks a lot for your patience and help!

[weiglszonja]

@oruebel Thank you for the suggestions, I completely agree with your comments and I hope now this PR is in the right shape. This codecov is still bugging me, maybe adding another test wouldn't hurt?

[oruebel]

This codecov is still bugging me, maybe adding another test wouldn't hurt?

I just added variants of the three relevant unit test cases that you had added to run the same test also in construct mode. I think that should take care of the codecov. codecov is I believe running the different tests suits (unit, integration, back_combat etc.) separately and my guess is that for some reason it is only considering the unit tests here. Adding tests in the unit test suit should, therefore, hopefully fix this.