Skip to content

Conversation

@TRuikes
Copy link

@TRuikes TRuikes commented May 28, 2020

Add info about inversion
Commented not-used lines in read_txt_header
In read_txt_header writes None to 'recording_opened' or 'recording_closed' if either breaks

TODO:
deal with ring buffer errors
deal with data of different lenghts (dsp)

follow up on #819
Data which cannot be read by current IO (or explaining the issues) are here

Commented not-used lines in `read_txt_header`
In `read_txt_header` writes None to 'recording_opened' or 'recording_closed' if either breaks
@pep8speaks
Copy link

pep8speaks commented May 28, 2020

Hello @TRuikes! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 40:100: E501 line too long (106 > 99 characters)
Line 442:100: E501 line too long (101 > 99 characters)
Line 512:100: E501 line too long (115 > 99 characters)
Line 514:100: E501 line too long (117 > 99 characters)
Line 515:21: E125 continuation line with same indent as next logical line
Line 516:100: E501 line too long (125 > 99 characters)
Line 521:100: E501 line too long (138 > 99 characters)
Line 524:100: E501 line too long (103 > 99 characters)
Line 580:100: E501 line too long (104 > 99 characters)
Line 584:100: E501 line too long (101 > 99 characters)
Line 588:100: E501 line too long (102 > 99 characters)
Line 591:28: E226 missing whitespace around arithmetic operator
Line 594:100: E501 line too long (109 > 99 characters)
Line 596:100: E501 line too long (102 > 99 characters)
Line 601:100: E501 line too long (105 > 99 characters)
Line 604:100: E501 line too long (101 > 99 characters)
Line 606:39: E226 missing whitespace around arithmetic operator
Line 606:100: E501 line too long (143 > 99 characters)
Line 606:102: E262 inline comment should start with '# '
Line 607:100: E501 line too long (131 > 99 characters)
Line 608:100: E501 line too long (102 > 99 characters)
Line 609:67: E226 missing whitespace around arithmetic operator
Line 610:78: E226 missing whitespace around arithmetic operator
Line 619:100: E501 line too long (100 > 99 characters)
Line 620:100: E501 line too long (100 > 99 characters)
Line 626:100: E501 line too long (100 > 99 characters)
Line 627:100: E501 line too long (100 > 99 characters)
Line 635:100: E501 line too long (100 > 99 characters)
Line 640:59: E226 missing whitespace around arithmetic operator
Line 642:100: E501 line too long (110 > 99 characters)
Line 1000:1: W391 blank line at end of file

Comment last updated at 2020-06-05 07:49:35 UTC

Thijs added 2 commits May 28, 2020 19:18
…hannels` to True show fix my problems, leaving it to false will return old behaviour.

Patching will make 1 segment out of the data, not recommended for data with large gaps.
…ill scan a directory for ncs files, detect misalignements/gaps in data and patches accordingly. Output is written in a new directory.

`read_ncs_files` is now unchanged
@TRuikes
Copy link
Author

TRuikes commented May 29, 2020

I've moved all the patching functionality for dealing with misalginment and gaps to a separate function, which we can call 1 time per dataset to generate a patched dataset. This way, if you don't think it should be in rawio, we can move it elsewhere. The output should be compatible with the IO as is.

@TRuikes
Copy link
Author

TRuikes commented May 30, 2020

I'm finished with the draft, for these two files it seems to work, I'm going to check on the other data we have in the future.

At this point the whole patching story is a separate function. I left it as a static method for convenience.

@apdavison apdavison added this to the 0.9.0 milestone Jul 3, 2020
info['recording_opened'] = datetime.datetime.strptime(
dt1['date'] + ' ' + dt1['time'], datetimeformat)
else:
info['recording_opened'] = None
Copy link
Contributor

Choose a reason for hiding this comment

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

good idea

Copy link
Member

Choose a reason for hiding this comment

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

This is an issue that could be addressed in a more general fashion in #851. @PeterNSteinmetz, what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, @JuliaSprenger, I think this is dealt with in #851, with a different pattern in the NlxHeader parsing code.

Copy link
Author

Choose a reason for hiding this comment

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

@PeterNSteinmetz nice update on the io, I think we had a few recording sessions where the recording software was closed too early. In this case there is no closing time in the header (but the keyword is there example file)

I fixed it with these lines, for our data.

Copy link
Contributor

Choose a reason for hiding this comment

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

@TRuikes I agree that is the right way to handle it and it is reasonable to allow the closing time to be optional generally. Would you create a pull request from that change?

@samuelgarcia
Copy link
Contributor

@TRRuikes : if I understand your PR you want be able to rewrite some some neuralynx files that contains gaps into mono-segment (without gap) new neuralynx folder.

I think that this would be very usefull in a generic (io inpedant) function.

For instance taking any rawio instance that have multi-segment signals and writte then into a raw binary IO files.

Having this functionality inside a rawio is a bit dangerous for long term maintenance because it is too specific.

@JuliaSprenger : what do you think ?

@JuliaSprenger
Copy link
Member

@samuelgarcia I agree, the patching functionality should rather go into some sort of utility function, but in a more generic fashion, such that it can be used across IOs.
@TRuikes: Also, if you have files exhibiting these irregularities, would you be willing to contribute one to the set of neo testfiles so we can cover this in the unittests?

@TRuikes
Copy link
Author

TRuikes commented Dec 21, 2020

hi @JuliaSprenger @samuelgarcia @PeterNSteinmetz sorry for the no reply.

I think I'll close this PR since it's on an old version of neo now, and you've implemented some nice features. I had uploaded the breaking data on gin, but something went wrong with a PR, so I tried again just now. It should contain a few files which were/are not readable by the io (the reason specified in the description)

There is still a header parsing bug for me, I commented the solution for my data above.

Your concatenate function works nice, the padding keyword makes it even better. The only problem now is that 'ringbuffer errors' (ie losing datapoints during recording) occurs per channel. So in the case of my example data, for 1 ncs file it might detect 8 segments, and for another it might detect 10 segments, which the current neo does not allow to read (this is a good thing, in the sense that the user is aware something is up). Since I won't be able to read the data, I also cannot use the concatenate function for individual signals to fix them. One solution would be to allow the Neuralynx Io to read a single file, and extend the Io with a 'writer'. This way I can loop over all individual channels, use concatenate and write to store a fixed version.

Finally this section might be a neat addition. It will notify the user if during recordings a lag in one of the channels arises due to digital filtering, and cheetah not correcting for it (a problem we had, an example of this in the data uploaded on gin).

@PeterNSteinmetz
Copy link
Contributor

@TRuikes I think the longer term solution for this lies in handling the multiple sections (which is a name I introduced in the source code changes for contiguous groups of blocks of records) in some standardized way. This could be either creating multiple analog signals for a given channel which has multiple sections, or splitting up the Neo segments. I lean toward the former solution presently as the Neo definition is simply a set of data sharing the same common clock, which is the case for these different sections. Analog signals could be named with their normal name and a suffix, like _sect0, _sect1, etc. How would that strike you for your use case?

@TRuikes
Copy link
Author

TRuikes commented Dec 22, 2020

It sounds good, I think the most important thing is that all the data is able to load and we are warned if something suspiscious happens.

@JuliaSprenger
Copy link
Member

Hi @TRuikes I think the main aim of this PR is already covered in the master branch now via #836, correct? I suggest we continue the discussion in #819 and close this PR. What do you think?

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.

6 participants