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

408 check out contiguous channel property for discrete data #415

Merged

Conversation

tensionhead
Copy link
Contributor

@tensionhead tensionhead commented Jan 10, 2023

Changes Summary

This PR does 2 things, but can't follow the original idea about contiguous channel labels

  • new attributes for SpikeData: .channel_idx and .unit_idx, holding the np.unique results
  • .channel/.unit property lookups are no longer re-computed and hence as fast as they should be
  • checks for integer data type of .data for DiscreteData

I hope I captured what you had in mind @kshapcott, here's an example how it works now:

spd = spy.SpikeData(data = 4 * np.ones((5,3), dtype=int), channel=['my_only_channel'])

# assigned label
spd.channel
>>> array(['my_only_channel'], dtype='<U15')

# there's only channel 4
spd.channel_idx
>>> array([4])

# default label
spd.unit
>>> array(['unit05'], dtype='<U6')

# there's only unit 4
spd.unit_idx
>>> array([4])

Note that both channel and unit use 0-indexing now, before only channel was 0-based for some reason.

Reviewer Checklist

  • Are testing routines present?
  • Do objects in the global package namespace perform proper parsing of their input?
  • Are all docstrings complete and accurate?
  • Is the CHANGELOG.md up to date?

Changes to be committed:
	modified:   syncopy/datatype/discrete_data.py
Changes to be committed:
	modified:   syncopy/datatype/discrete_data.py
- it's probably confusing to have non-existent channels after a selection

Changes to be committed:
	modified:   syncopy/datatype/discrete_data.py
- now also selections are very fast

Changes to be committed:
	modified:   syncopy/datatype/discrete_data.py
Changes to be committed:
	modified:   syncopy/datatype/discrete_data.py
@tensionhead tensionhead linked an issue Jan 11, 2023 that may be closed by this pull request
- either initialize with `data=None`, or `data.size != 0`

Changes to be committed:
	modified:   syncopy/datatype/discrete_data.py
	modified:   syncopy/tests/test_discretedata.py
Changes to be committed:
	modified:   syncopy/datatype/discrete_data.py
- weird that this worked in the 1st place..

Changes to be committed:
	modified:   syncopy/tests/test_basedata.py
@codecov
Copy link

codecov bot commented Jan 17, 2023

Codecov Report

Base: 68.22% // Head: 68.37% // Increases project coverage by +0.15% 🎉

Coverage data is based on head (992e177) compared to base (5816ae2).
Patch coverage: 65.07% of modified lines in pull request are covered.

❗ Current head 992e177 differs from pull request most recent head 2917162. Consider uploading reports for the commit 2917162 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #415      +/-   ##
==========================================
+ Coverage   68.22%   68.37%   +0.15%     
==========================================
  Files          80       80              
  Lines        9602     9629      +27     
  Branches     1993     2007      +14     
==========================================
+ Hits         6551     6584      +33     
+ Misses       2527     2523       -4     
+ Partials      524      522       -2     
Impacted Files Coverage Δ
syncopy/datatype/discrete_data.py 77.81% <65.07%> (-1.89%) ⬇️
syncopy/plotting/_helpers.py 85.91% <0.00%> (-2.82%) ⬇️
syncopy/plotting/mp_plotting.py 44.66% <0.00%> (-1.34%) ⬇️
syncopy/datatype/base_data.py 76.72% <0.00%> (-0.39%) ⬇️
syncopy/datatype/util.py 71.64% <0.00%> (+1.49%) ⬆️
syncopy/specest/compRoutines.py 92.49% <0.00%> (+2.38%) ⬆️
syncopy/specest/freqanalysis.py 68.99% <0.00%> (+3.64%) ⬆️
syncopy/specest/mtmconvol.py 92.59% <0.00%> (+7.40%) ⬆️
syncopy/specest/stft.py 81.08% <0.00%> (+8.10%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

- it's tricky due to empty data class inits

Changes to be committed:
	modified:   syncopy/datatype/discrete_data.py
Changes to be committed:
	modified:   syncopy/datatype/discrete_data.py
@tensionhead tensionhead linked an issue Jan 17, 2023 that may be closed by this pull request
- to be in sync with channel

Changes to be committed:
	modified:   syncopy/datatype/discrete_data.py
Changes to be committed:
	modified:   syncopy/tests/test_discretedata.py
@tensionhead tensionhead added the Performance Improve the number crunching label Jan 17, 2023
@tensionhead tensionhead marked this pull request as ready for review January 17, 2023 16:00
Changes to be committed:
	modified:   syncopy/tests/test_basedata.py
	modified:   syncopy/tests/test_spyio.py
Changes to be committed:
	modified:   syncopy/datatype/discrete_data.py
	modified:   syncopy/tests/test_discretedata.py
@tensionhead
Copy link
Contributor Author

Ok.. now the coverage complains about lines I haven't touched, so I won't grind it any further for the moment

Copy link
Contributor

@KatharineShapcott KatharineShapcott left a comment

Choose a reason for hiding this comment

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

Looks good! Should be soooo much faster for my data in combination with 403. I can make those changes if you agree.

@tensionhead
Copy link
Contributor Author

Looks good! Should be soooo much faster for my data in combination with 403. I can make those changes if you agree.

Thx, yes please go ahead with the little changes you proposed 👍

@tensionhead
Copy link
Contributor Author

tensionhead commented Jan 18, 2023

Ok, thx for the review and the final fix ups! Shall I merge it with my super powers?

EDIT: I think you would need to do a final 1 sentence approving review if we wanna get rid of some of the red here 😀

Copy link
Contributor

@KatharineShapcott KatharineShapcott left a comment

Choose a reason for hiding this comment

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

Approved

@KatharineShapcott
Copy link
Contributor

Wait but aren't we still failing the code coverage check?

@tensionhead
Copy link
Contributor Author

Wait but aren't we still failing the code coverage check?

yes, but this is not entirely reliable sadly.. we merged things in that state before ;)

@tensionhead tensionhead merged commit f7664f9 into dev Jan 18, 2023
@tensionhead tensionhead deleted the 408-check-out-contiguous-channel-property-for-discrete-data branch January 18, 2023 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Performance Improve the number crunching
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check out contiguous channel property for discrete data Don't accept non-integers for DiscreteData
2 participants