NeuralEnsemble/python-neo — pull request #163 by mdenker (2014-05-28 07:19:36+00:00)
Title: New BlackrockIO interface

This revised pull request (original pull request 140) adds a new implementation of the IO for Blackrock files. The IO extends the functionality of the previously available one by allowing to read also spike and event data besides analog signal data. The main functionality not yet implemented in the rewrite is the ability to write Blackrock files.

The previous IO was moved to blackrockio_deprecated.py, for compatibility reasons, and until write functionality is integrated into the new IO.

The IO comes with two IO specific unit tests based on a data set provided by Blackrock Microsystems (uploaded to GNode portal with permission). One tests for input parameter handling, and one compares the loaded data to data loaded using a matlab implementation provided by Blackrock Microsystems.

The following items of former pull request 140 have not yet been addressed (digest, see pull request 140 for details):
* could you remove if name == 'main': at the begining
* Could you fix the line width <80
* read_params is empty so with GUI like OpenElectrophy. read_block parameters won't be available in the user dialog. Maybe this is not important at the moment.
* why using this LFPunit = pq.CompoundUnit('10^-6*V') and not this LFPunit = 'uV' ?
* If implemented n_start and n_stop for selecting a chunk in signals. In a next futur this could be part of the officiel neo.io API and the mechanism could change and could be more formal but this is good for the moment.
* is the waveforms a 3D array ?
* Probably "extensions = ['ns' + str(_) for _ in range(1, 7)] + ['nev']" is clearer
* Looking in more detail, this seems to violate the "one io class per extension" rule.
* Also, a lot of implementation details are exposed as public attributes. At least in my opinion, these implementation details should be private, and only the standard neo API should be public
* I wouldn't expose diagnostic_print via the public API. [ ... ]So in this situation, I think it would be better to just prefix the attributes with a single underscore "_" and describe them in comments in the code.
* "if fileheader == 'NEURALSG':" This approach becomes difficult to manage if more versions are added in the future. I think it would be better to have one method per version, then use a dict of fileheader:method pairs to select the appropriate one. This makes it much easier to add new versions in the future.
* Same applies for other block-checks
* If only loading a subset of the data is an important usecase, I would encourage you to include the relevant parameters in `read_parameters` as I have previously suggested, so that tools like OpenElectrophy and Spyke Viewer can use this feature.
* I don't think initalizing the IO class should load any data from inside the files, even metatadata. I would opt for a solution where there is a private method that reads the metadata. read_block checks if this has been called already and, if not, calls it. It is still a method, though, so it can be called manually as well for someone who wants to use the private internals.
* Please do not use mutable types (i.e. lists) as default arguments here (or pretty much anywhere...).
* User create_many_to_one_relationship. Small correction: Since you are working one the 0.4.0 candidate branch (maybe this shouldn't be master...), create_many_to_one_relationship is no longer in neo.tools, but a method of the block itself.