[WIP] Proxy object implementation and new lazy mode implementation.

[samuelgarcia]

@JuliaSprenger : Proof of concept for discussion.
Very first comit.

[pep8speaks]

Hello @samuelgarcia! Thanks for updating the PR.

• In the file neo/io/proxyobjects.py, following are the PEP8 issues :

Line 218:21: E126 continuation line over-indented for hanging indent
Line 218:100: E501 line too long (107 > 99 characters)
Line 290:100: E501 line too long (108 > 99 characters)
Line 384:100: E501 line too long (118 > 99 characters)
Line 395:100: E501 line too long (103 > 99 characters)
Line 403:100: E501 line too long (102 > 99 characters)
Line 501:5: E722 do not use bare 'except'

• In the file neo/test/iotest/common_io_test.py, following are the PEP8 issues :

Line 97:37: W504 line break after binary operator
Line 100:36: W504 line break after binary operator
Line 164:21: W504 line break after binary operator

• In the file neo/test/iotest/test_blackrockio.py, following are the PEP8 issues :

Line 34:33: W504 line break after binary operator
Line 35:33: W504 line break after binary operator

Comment last updated on January 14, 2019 at 14:56 Hours UTC

[coveralls]

Coverage decreased (-0.2%) to 49.014% when pulling 17a6c55 on samuelgarcia:lazy_load_with_proxy into 7fed9ec on NeuralEnsemble:master.

[coveralls]

Coverage decreased (-0.2%) to 49.014% when pulling 27d79ed on samuelgarcia:lazy_load_with_proxy into 7fed9ec on NeuralEnsemble:master.

[coveralls]

Coverage increased (+0.3%) to 48.364% when pulling 0bafcde on samuelgarcia:lazy_load_with_proxy into 078912b on NeuralEnsemble:master.

[samuelgarcia]

@apdavison @JuliaSprenger : the proof of concept for AnalogSignalProxy is almost done.

Could you have a very short look ?
I need feedback, so I could go on with EventProxy/Epoch/SpikeTrainProxy.

The easier to anderstand the logic is to read tests : neo/test/iotest/basefromrawio/test_proxyobjects.py

When all the proxy will be done I will be able to rewrite enterily the BaseFromRaw with a very simpler code and the new lazy mode system.

I known that it is was not in the top priority but the sooner we have this the less we will need to go back to each IO when cleanning.

[samuelgarcia]

@JuliaSprenger @apdavison: except my new friend pep8 proxy object themself are done.

BaseFromRaw do not use them for the moment.

My plan is :

• release 0.6 as soon as possible in february wihtout proxy object of course.
• modify BaseFromRaw and change to the new lazy mode (that give proxy object)
• integrate this in master soon

This would mean that proxy object would be in 0.7 alongside with array_annotations.

What do you think ?

[JuliaSprenger]

Technically I don't see any problem in introducing proxy objects and array_annotations at the same time. However, I think in the original plan we wanted to combine the major changes (object model + proxy object) in version 0.8, since then 0.7 would still be (more or less) compatible with 0.6.

[samuelgarcia]

Yes it was the original plan we discuss.
But since the proxy object mechanism is almost done my idea was to push it into master sooner.
0.6 and 0.7 still would be ciompatible except for the lazy mode.
It is just a proposal.

[samuelgarcia]

@JuliaSprenger @apdavison : I have implemented the new lazy=True.
This now give proxy objects.

I have put some example in doc.

If someone have time for a global comments, you should start with BaseFromRaw class that have been simplified a lot.

Note that **magnitude_mode='raw'/'rescale' will be done in a separate PR because it need and extention of rawio API and so modification in all RawIO classes.

pep8 will be done!!

[samuelgarcia]

@JuliaSprenger @mdenker @bjoern1001001 :
A bugy slice is when t_start or t_stop is outside the recording.
proxyobject load(time_slice=buggy_slice) is not allowed.
I think this is a correct behavior.

BaseFromRaw.read_segment(time_slice=buggy_slice) use to be allowed at least in 0.6 for many IOs (and in 0.5.x for Blackrock). I am not very fan of this feature, I would prefer a clear error. If you still need to support buggy_slice, we can make a hack in BaseFromRaw.read_segment (aka transform the slice if buggy) but I prefer to avoid to do it directly in proxyobjects.

In current PR, neo/test/iotest/test_blackrockio.py trig an error in cricle-ci because bugy_slice is supposed to work and it is not the case for now.

[samuelgarcia]

@JuliaSprenger : in blackrock I have a new error in circle-ci that has nothing to do with this PR.
There is a mismatch between the matlab waveform size and the neo wavzform size.

This occur only for one units and for one version file (2.1).
I am not sur but my guess is that in this

    def __get_waveform_size_variant_a(self):
        """
        Returns wavform sizes for all channels for file spec 2.1 and 2.2
        """
        wf_dtypes = self.__get_waveforms_dtype()
        nb_bytes_wf = self.__nev_basic_header['bytes_in_data_packets'] - 8
        wf_sizes = dict([
            (ch, int(nb_bytes_wf / np.dtype(dt).itemsize)) for ch, dt in
            wf_dtypes.items()])
        return wf_sizes

the -8 could be not accurate
Since there is a integer division is very often OK but for one file it is not.
Unless the error is in the matlab convertor.
It is just a guess of course.

I am wondering why this do not raise error before ?!!???

If you have time could you check ?

[samuelgarcia]

And the most strange test fail is this one:

======================================================================
FAIL: This test compares the output of BlackrockIO.read_block() with the
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/samuel/Documents/projet/python-neo/neo/test/iotest/test_blackrockio.py", line 305, in test_compare_blackrockio_with_matlabloader_v21
    assert_equal(np.atleast_2d(np.squeeze(st_i.waveforms).magnitude), matlab_wf)
  File "/home/samuel/.virtualenvs/neodev/lib/python3.6/site-packages/numpy/testing/_private/utils.py", line 347, in assert_equal
    return assert_array_equal(actual, desired, err_msg, verbose)
  File "/home/samuel/.virtualenvs/neodev/lib/python3.6/site-packages/numpy/testing/_private/utils.py", line 865, in assert_array_equal
    verbose=verbose, header='Arrays are not equal')
  File "/home/samuel/.virtualenvs/neodev/lib/python3.6/site-packages/numpy/testing/_private/utils.py", line 740, in assert_array_compare
    raise AssertionError(msg)
AssertionError: 
Arrays are not equal

(shapes (111, 48), (112, 48) mismatch)
 x: array([[  0.,   1.,  -1., ...,  -5., -11., -17.],
       [  7.,   5.,   9., ...,  -5.,   1.,  -1.],
       [  3.,  -1.,   4., ...,  -4., -12., -23.],...
 y: array([[  0,   1,  -1, ...,  -5, -11, -17],
       [  7,   5,   9, ...,  -5,   1,  -1],
       [  3,  -1,   4, ...,  -4, -12, -23],...

waveform shape is not the same as in matlab version.

Could it be a time limit ?

EDIT : this is fixed.

[JuliaSprenger]

@samuelgarcia: Regarding the buggy_slice my feeling is raising an error here will break a lot of code or requires explicit time range checking each time slicing is used. That's why I would vote for having the accept_buggy_slice keyword. An alternative naming idea might be something like exact_slicing or strict_slicing or reversely flexible_slicing.

[samuelgarcia]

OK I will add this keyword.

My favorite is strict_slicing.

For the default behavior, idealy it would be strict_slicing=True always.
But it will break some of your codes so I propose:

• in proxy object in ProxyObject.load(strict_slicing=True) never used before so no compatibility
  problems
• in BaseFromRaw.read_segment(strict_slicing=False) with a warning that indicate that the default
  behavior will change in next version

What do you think ?

[JuliaSprenger]

strict_slicing sounds like a good choice!
Since the keyword strict_slicing also didn't exist before for BaseFromRaw.read_segment() I don't think you have to wait to change the default behaviour. Existing code anyway needs to be adjusted to the new keyword. So you can use BaseFromRaw.read_segment(strict_slicing=True) from the beginning.

[samuelgarcia]

Done.
And the good news is that test_blacrockio.py do not raise error .
Yeah!!

[JuliaSprenger]

I am surprised the tests are not failing due to this:

You still need to add the neo.io.basefromrawio here

python-neo/setup.py

Line 34 in 82665b9
'neo.rawio', 'neo.rawio.tests'],

Locally I need to fix this before I can import eg axonio from an installed version of neo. How is circleci doing this?

[samuelgarcia]

Yep. This is strange, I must be lucky.
Maybe circleci do not import the installed one but the one in working directory.
And this is bad because we can't track this kind of errors.

About this, @apdavison mentioned that he would prefer avoid this subdirectory.

Do I move back:

• neo/io/basefromraw/base.py to neo/io/basefromraw.py
• neo/io/basefromraw/proxyobjects.py to neo/io/proxyobjects.py
  or do I modify the setup.py ?

What do you think ?

[JuliaSprenger]

I am happy with both solutions, so we can follow @apdavison.
Regarding CircleCI, maybe there is a possibility to change the working directory before running the tests?

[JuliaSprenger]

The sampling_period is also still missing for AnalogSignals: #461 (comment)

[samuelgarcia]

OK.
I have removed the subdirectory.

@JuliaSprenger @apdavison @mdenker
In my mind, this PR is done.
If you have time please make some tests.
I would be in favor in merging this soon in the master.

Then, as we already discuss in codejam, we would work independently on:

• raw magnitude from spiketrain times. This need some modification on all rawios.
• incorporating array_annotation in more IOs and at more places.
• add a method in rawio that enable to read spike with a two columns (timestamp+label), so we will
  be ready for the next object model.

[apdavison]

Any preference for "merge" vs "squash-and-merge"?

[samuelgarcia]

Hi Andrew.
Thank you for reading.
No preference.

[apdavison]

ok folks, the future is now!

[samuelgarcia]

Champagne.