Skip to content

Conversation

@PeterNSteinmetz
Copy link
Contributor

Factors out parsing of information from header of file. Add determination of recording type from header information.

Changes indexes to indices in argument names again -- will need determination of acceptability of this spelling change for files being worked on only. See https://grammarist.com/usage/indexes-indices/ for discussion of usage, particularly in technical contexts.

This factorization is in preparation for larger changes to accommodate early file styles with different header structures. Provided here as a smaller chunk in order to avoid very much larger changes which are harder to check and understand.

@pep8speaks
Copy link

pep8speaks commented Oct 9, 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-11-06 21:42:25 UTC

@apdavison
Copy link
Member

Many thanks for this. A quick comment on the "indices/indexes" question. Having grown up in the UK, I prefer "indices" in general. However, we decided to use "indexes" in Neo because it is less confusing for non-native English speakers and more familiar to North Americans. It's fine to use "indices" for internal variables, but any arguments that are exposed to Neo users as part of the API should be "indexes".

@PeterNSteinmetz
Copy link
Contributor Author

A quick comment on the "indices/indexes" question. Having grown up in the UK, I prefer "indices" in general. However, we decided to use "indexes" in Neo because it is less confusing for non-native English speakers and more familiar to North Americans. It's fine to use "indices" for internal variables, but any arguments that are exposed to Neo users as part of the API should be "indexes".

So how about the case of the argument to _get_analogsignal_chunk ? That is nominally a private method but I imagine a lot of people use that. Should those also be 'indexes' ?

@PeterNSteinmetz
Copy link
Contributor Author

circleci is complaining that the test is failing because trying to download the file https://web.gin.g-node.org/NeuralEnsemble/ephy_testing_data/raw/master/neuralynx/Cheetah_v4.0.2/original_data/CSC14_trunc.Ncs is returning a 404 error.

Yet that file is available and downloadable in a browser or if I pull it using git from the gin-g repository.

What is going wrong there?

@JuliaSprenger
Copy link
Member

Hi @PeterNSteinmetz, I restarted the tests and the files could now be successfully downloaded from gin. It seems this was a temporary issue with gin. The tests are succeeding now.

@JuliaSprenger
Copy link
Member

A quick comment on the "indices/indexes" question. Having grown up in the UK, I prefer "indices" in general. However, we decided to use "indexes" in Neo because it is less confusing for non-native English speakers and more familiar to North Americans. It's fine to use "indices" for internal variables, but any arguments that are exposed to Neo users as part of the API should be "indexes".

So how about the case of the argument to _get_analogsignal_chunk ? That is nominally a private method but I imagine a lot of people use that. Should those also be 'indexes' ?

Yes, I think we should stay consistently with channel_indexes for now and discuss a potential name change in a separate issue. Could you please also use indexes for _get_analogsignal_chunk?

@PeterNSteinmetz
Copy link
Contributor Author

I will merge my NlxFixes branch into this PR, the TypesFromHeader branch, thereby fixing the channel_indexes question, once PR #851 is merged onto main. I want to leave NlxFixes open for now until that happens.

@JuliaSprenger
Copy link
Member

#851 is now merged in master and here. Are you planning to address the todos introduced in this PR also here or is this PR ready for review / merge from your side?

@PeterNSteinmetz
Copy link
Contributor Author

That is it for this branch. Please merge into master.

@PeterNSteinmetz
Copy link
Contributor Author

This is ready for merging.

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 a few naming issues I found, but otherwise the PR looks ready to merge to me. Thanks!

@PeterNSteinmetz
Copy link
Contributor Author

I think tests are still failing on hdf5io .

@PeterNSteinmetz
Copy link
Contributor Author

OK, just merged NeuralEnsemble:master back into my TypeFromHeader branch. Let's see how this goes.

@JuliaSprenger JuliaSprenger merged commit e97ac97 into NeuralEnsemble:master Nov 7, 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.

4 participants