Skip to content

Conversation

@MarinManuel
Copy link

In the CED documentation for the SON library, it says:

In version 9 and above files, if pos is of type TDOF, the disk offset is pos*DISKBLOCK. In version 8 and previous files, the disk offset is pos.

this PR fixes the offsets for v9 files, and fixes #818

@samuelgarcia
Copy link
Contributor

Hi @MarinManuel. This is super cool and should help many users.
Thanks a lot.

@samuelgarcia
Copy link
Contributor

I have push the file "Two-mice-bigfile-test000.smr"
See https://gin.g-node.org/NeuralEnsemble/ephy_testing_data/src/master/spike2

Could add it here in neo/test/rawiotest and here in neo/test/iotest and check that test are passing ?

@JuliaSprenger
Copy link
Member

The tests are failing as channel_names are expected to be of type 'S' and not unicode. @samuelgarcia was there a reason for this not to be unicode?
There are two options now:

  1. change the definition of the ChannelIndex class and make channel_name unicode. This affects all ios based on the raw mechanism.
  2. Hackily reading the channel_names as unicode and converting it to type S.

I would go for approach 2, since the ChannelIndex object will anyway be replaced in the future (#817), so we don't need to introduce unnecessary changes for ChannelIndex objects now.

@MarinManuel
Copy link
Author

@JuliaSprenger @samuelgarcia I've added a line to convert back the unicode strings to ascii for the time being. Hopefully that does not introduce any more problems.

@samuelgarcia
Copy link
Contributor

Hi all.
@JuliaSprenger this is strange. I thought we fix this old (and very anoying) behavior where everything was ascii string because of python 2. Now python27 is no more here and evrything should be unicode everwhere.

@MarinManuel : I propose to fix this str vs unicode problem outside this PR very very soon. So you could rebase this PR with the offcial fix. is it OK ?

@JuliaSprenger
Copy link
Member

@MarinManuel the format change is in master now, could you rebase and undo your changes in basefromrawio.py?


# Store recommended attributes
self.channel_names = np.array(channel_names)
self.channel_names = np.array(channel_names, dtype='S')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this. this is now fixed.

# at this time ch_names are expected to be type 'S'.
# see https://github.com/NeuralEnsemble/python-neo/pull/824#issuecomment-644295081
ch_names = [name.encode('ascii', errors='replace') for name in ch_names]
neo_channel_index = ChannelIndex(index=ind_within,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this. this is now fixed.

@samuelgarcia
Copy link
Contributor

@JuliaSprenger :Could we first merge this PR before #775 which is also related to spike2io ?

@JuliaSprenger
Copy link
Member

Sure, then we keep #775 in the queue.

@samuelgarcia
Copy link
Contributor

In fact I ask exactly the contary I wanted.
I wanted to say:
"Could we merge first #775 (mine older) and then #824 (this one)"

Sorry. One day I will read what I am writting before push the green button.

@MarinManuel
Copy link
Author

@samuelgarcia Hi, I have reverted the commit on files channelindex.py and basefromrawio.py, is that good enough? Bit of a git newbie here...

@JuliaSprenger
Copy link
Member

Hi @MarinManuel,
there are still conflicts between the current master branch on Neuralensemble and the master branch on your fork. What you can do is rebase your branch on top of the current Neuralensemble master. First make sure you also have an up-to-date local version of that one on your machine by running
git fetch upstream (assuming upstream points to Neuralensemble)
Then you can do the rebasing via
git rebase upstream/master - this will lead to the conflicts pointed out above to appear during the rebase in your local files. The conflicts are indicated in the files as

<<<<
first set of changes
=======
second set of changes
>>>>>

Please edit the files to include all the changes that are suggested by Neuralensemble master and additionally keep the ones you want to include in this PR.
Finally you add the files using git add <filename> and run git rebase --continue to finalize the rebase process.
To update this PR you then push the changes you made to your fork: git -f push origin master (assuming origin points to your fork). Here you need the -f flag as you want to overwrite the history of the commits in your fork, which were changed during the rebase process.

I hope this helps. Tell us if there's issues during the process.

@MarinManuel
Copy link
Author

Hi @JuliaSprenger
Thank you for the detailed explanations. Hope this works now

@apdavison apdavison added this to the 0.9.0 milestone Jul 3, 2020
@samuelgarcia
Copy link
Contributor

Hi @MarinManuel
could you modify theses two files like this:

neo/test/rawiotest/test_spike2rawio.py

class TestSpike2RawIO(BaseTestRawIO, unittest.TestCase, ):
    rawioclass = Spike2RawIO
    files_to_download = [
        'File_spike2_1.smr',
        'File_spike2_2.smr',
        ...
        'Two-mice-bigfile-test000.smr', # <<<<<< new line
    ]
    entities_to_test = files_to_download

neo/test/iotest/test_spike2io.py

class TestSpike2IO(BaseTestIO, unittest.TestCase, ):
    ioclass = Spike2IO
    files_to_test = [
        'File_spike2_1.smr',
        'File_spike2_2.smr',
        ...
        'Two-mice-bigfile-test000.smr', # <<<<<< new line

And afte rthis I will be able to merge this PR.

thanks again.

@MarinManuel
Copy link
Author

could you modify theses two files like this:
@samuelgarcia yes, done

@samuelgarcia
Copy link
Contributor

thanks.
merging now.

@samuelgarcia samuelgarcia merged commit 3ed56f8 into NeuralEnsemble:master Jul 6, 2020
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.

SpikeIO does not load .smr

5 participants