Skip to content

Conversation

@PeterNSteinmetz
Copy link
Contributor

This set of changes fixes a bug which prevented the Pegasus .Nev event only files from passing the test suite.

It also allows filename extensions to be in any letter case. For the InputInverted header parameter to be missing,
meaning not inverted. Checks for 8 hashes at the start of the header as a check on header contents. Allows
any whitespace between a header key and value.

Internal changes are addition of a large number of explanatory comments and refactoring of the code to handle
the header into a separate class. Many of these in preparation for handling earlier versions of neuralynx file headers
and more flexible handling of segments withins .Ncs files.

@pep8speaks
Copy link

pep8speaks commented Sep 3, 2020

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

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-10-28 15:42:23 UTC

@apdavison
Copy link
Member

Many thanks for this PR. Please also add your name and affiliation to doc/source/authors.rst

@apdavison apdavison added this to the 0.9.0 milestone Sep 7, 2020
@samuelgarcia
Copy link
Contributor

Hi Peter,
thank you very much for this.
Except the comment I have made in the code, I think I am OK with this patch.
Lets see if Andrew or Julia will have time to review this.
Cheers
Samuel

@PeterNSteinmetz
Copy link
Contributor Author

It is definitely easy to revert the change of the local argument name to channel_indices if that is what people want. Since this is a name simply local to the function and makes no difference to the caller, I would suggest just gradually changing this spelling in functions as they are changed for other reasons.

@apdavison apdavison added the IO label Sep 24, 2020
@JuliaSprenger JuliaSprenger self-assigned this Sep 24, 2020
@PeterNSteinmetz
Copy link
Contributor Author

Bumping this. Can I get some advice on use of channel_indices for local argument names in non-public functions, i.e., those beginning with _ ? I can then change this in both of my current pull requests.

@JuliaSprenger
Copy link
Member

Hi @PeterNSteinmetz, thanks for the the PR . Look good to me. Please just revert the name changes for channel_indices and I would be happy to merge.

Copy link
Member

@JuliaSprenger JuliaSprenger left a comment

Choose a reason for hiding this comment

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

There are still a number of TODO notes in your changes. Are you planning to address these in this PR, i.e. is this PR still work in progress?

The use of channel_indexes is a standard throughout neo at present.
@PeterNSteinmetz
Copy link
Contributor Author

Appears now to past all tests and should be ready to merge.

@JuliaSprenger JuliaSprenger merged commit 0d4ab3d into NeuralEnsemble:master Oct 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants