Skip to content
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

Electrodes mapping #92

Merged
merged 6 commits into from
Jun 10, 2020
Merged

Electrodes mapping #92

merged 6 commits into from
Jun 10, 2020

Conversation

luiztauffer
Copy link
Member

@codecov
Copy link

codecov bot commented Jun 8, 2020

Codecov Report

Merging #92 into master will decrease coverage by 0.00%.
The diff coverage is 10.25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #92      +/-   ##
==========================================
- Coverage   19.12%   19.12%   -0.01%     
==========================================
  Files          25       25              
  Lines        5584     5580       -4     
==========================================
- Hits         1068     1067       -1     
+ Misses       4516     4513       -3     
Flag Coverage Δ
#unittests 19.12% <10.25%> (-0.01%) ⬇️
Impacted Files Coverage Δ
ecogvis/ecogvis.py 11.38% <0.00%> (-0.02%) ⬇️
ecogvis/functions/subFunctions.py 8.40% <2.85%> (-0.07%) ⬇️
ecogvis/functions/nwb_copy_file.py 67.34% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9935920...5667c0e. Read the comment docs.

@luiztauffer luiztauffer requested a review from bendichter June 8, 2020 09:24
@luiztauffer luiztauffer marked this pull request as ready for review June 8, 2020 09:24
@@ -266,10 +266,10 @@ def copy_obj(obj_old, nwb_old, nwb_new):

# ElectricalSeries --------------------------------------------------------
if type(obj_old) is ElectricalSeries:
nChannels = len(obj_old.electrodes.table['x'].data)
elecs_region = nwb_new.electrodes.create_region(
region = list(obj_old.electrodes.to_dataframe().index)
Copy link
Contributor

Choose a reason for hiding this comment

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

Here you are converting to a dataframe, which will work but is unnecessarily costly if all you want is the ids in the region. It is more efficient to first get the ids from the table, and then index them by the data field of the ElectricalSeries.electrodes.data. See the eventual fix for similar code in SpikeInterface:

https://github.com/SpikeInterface/spikeextractors/pull/401/files

self.nChTotal = self.source.data.shape[1] # total number of channels
self.allChannels = np.arange(0, self.nChTotal) # array with all channels
self.all_channels_ids = list(self.source.electrodes.to_dataframe().index) # all channels ids
self.n_channels_total = len(self.all_channels_ids) # total number of channels
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar issue here. See:

https://github.com/SpikeInterface/spikeextractors/pull/401/files

Also, I don't the "all_channel_ids" is an appropriate name here because we are intentionally excluding the channels that are not associated with the ElectricalSeries, so maybe electrical_series_channel_ids would be better

ecogvis/functions/subFunctions.py Show resolved Hide resolved
ecogvis/functions/subFunctions.py Outdated Show resolved Hide resolved
- remove unused button save bad channels
- change channel index in copy
@bendichter bendichter merged commit f2d9eff into master Jun 10, 2020
@luiztauffer luiztauffer deleted the electrodes_mapping branch June 10, 2020 13:32
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.

Cannot save bad channels
2 participants