-
Notifications
You must be signed in to change notification settings - Fork 23
Add type hints and tests to analysis
package
#43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
joschaschmiedt
wants to merge
32
commits into
open-ephys:main
Choose a base branch
from
joschaschmiedt:add-tests
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- The untyped metadata dicts were replaced with generalized dataclasses SpikeMetadata and ContinousMetadata - Typt hints were gradually introduced in different file formats.
- Recorded with File Reader and Network Events - plugin-gui v0.6.7
+ some fixes here and there
The json schema can be used to validate the structure of a given oebin file.
Thanks for this! Let us know when you're finished making changes and we will test it out. |
analysis
package
@jsiegle, I think I'm done with changes from my side. I'd be happy if you could test it out. |
analysis
packageanalysis
package
Thanks, we should be able to take a closer look at this next week. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While exploring the API of the analysis package, I saw that there is an implicit abstraction of the
Continuous
andSpikes
classes. I made that abstraction explicit (addingContinuousAbstract
andSpikesAbstract
interfaces) and added type hints throughout theanalysis
package.I've also added test datasets for the binary, open-ephys and NWB data formats as well as test cases for reading those data in. While doing so I fixed potential bugs
float64
dtype (see ecd427b)Spikes
from the open-ephys format test dataset causes a reshaping error (seetest_spike_data
intest_openephys_format
).BinaryRecording
contained a call to the non-existent functioncreate_channel_map
If you find any of this useful, feel free to merge and modify my suggestions.Here's the complete CHANGELOG:
Continuous
andSpike
classes of the three formats now have an explicit interface(i.e. abstract parent class) and have been renamed to
BinaryContinuous
,BinarySpike
etc.Continuous
andSpike
in the analysis package now are typed dataclassesinstead of
dict
objects . This makes accessing metadata more reliable.analysis
package.RecordingFormat
enum for the three formatsuv.lock
file for reproducible development environments.BinaryContinuous
andBinarySpike
now have__str__
methods to give an overview overtheir contents.