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.
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
[ENH, MRG] Add EpochsSpectrumArray and SpectrumArray classes #11803
[ENH, MRG] Add EpochsSpectrumArray and SpectrumArray classes #11803
Changes from 7 commits
395a758
710cef4
e34bc38
3fcb557
597c48c
6a54f68
125903e
e0fb07f
8cee941
738c8d3
0cd6ec8
5d18ff3
27be8d8
145b6bb
71016d6
1ccf8a4
ece2b39
ff203ff
725446d
ecbd0a8
f8e98ce
cb80688
fd830e9
e5bd6a9
e1bcbd9
4a76c9f
7ef0626
f031c8b
62e25f4
f8e74c9
36df9b5
147d110
ba24ebb
026e8e6
7340ea2
e4ca8ca
4d48f42
b98d2b8
d8fefc8
3c4da24
9c2cd25
10a2667
92d8a9d
5a33651
6404aaa
da45d5c
1a42bf6
e71672a
04262f2
b099177
265f009
2427b9f
54488ed
1a1e57f
9f45402
8f49d05
831c1ee
b6707f3
e178231
88c790e
f735e12
40627ac
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the data comes from an array we don't know if it was computed on Raw or averaged (Evoked) data. Also we don't know if it's power/amplitude, real/complex, etc.
This will probably break some tests (assuming we're testing thoroughly) so hopefully those test failures can guide what needs to change elsewhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So real/complex can be inferred from the data type. We ask specifically for power in the constructor so maybe we should require that and let the
amplitude
kwarg inSpectrum.plot
handle that? That'd be my 2 cents but not highly opinionated, whatever is simplestThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wondering what makes the most sense for these. Would like opinions of other devs (@hoechenberger @larsoner @mscheltienne @agramfort) as well as @alexrockhill... Off the top of my head:
events
I can see why folks might want to pass this in but is there any reason it needs to be a "proper" events array (N x 3)? Seems like only the last column is going to matter? it can be set after-the-fact so we could omit it from the constructor, but my gut is that this will be frequently used so maybe best to leave it here?event_id
again I can see why folks might want this (for__getitem__
)... it can be set after-the-fact and probably doesn't hurt to allow that; not sure about forcing it to be after-the-fact if we're going to haveevents
as a param though. Thoughts?metadata
can be set after instantiation so I'm inclined to leave it out of the constructorselection
seem like this shouldn't be needed if you're working withnp.array
data. Is there a use case you're thinking of where users would want to pass anything besidesselection=np.arange(data.shape[0])
? If not we can just set that internallydrop_log
similar toselection
; is there a use case you're thinking of where users would want to pass anything besidesdrop_log=tuple(tuple() for _ in range(data.shape[0]))
? If not we can just set that internallyIf we do end up keeping
events
andevent_id
as-is, the docstring should be deduped withEpochsArray
(where it seems you copy-pasted these entries from)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 on simplifying the constructor, as inferred, this was copy-pasted with the thought that it should just be similar to
EpochsArray
and without much more thought than that. I thinkevents
andevent_id
are helpful in the constructor so people don't lose expected functionality.