-
Notifications
You must be signed in to change notification settings - Fork 13
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
[WIP] add frequency API and compute coffeine API #51
Conversation
coffeine/power_features.py
Outdated
return frequency_bands | ||
|
||
|
||
def make_coffeine_df(C: np.ndarray, |
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.
@apmellot @antoinecollas @apmellot @DavidSabbagh @hubertjb @bmalezieux
A small but important & overdue function :) What do you think about its naming? I think I am in principle happy with it. Other intuitive options would be to call it make_coffeine_data_frame
or make_covariance_data_frame
. I would be OK with making it explicit (data_frame
rather than df
). Slight preference for coffeine make_coffeine_data_frame
for branding and energizing weirdness. I understand make_coffeine_data_frame
could be more boring and predictable (in the dispassionate scientific sense), which would be not bad either.
Any thoughts? (for the bigger picture, please take minute to zoom out and look at the entire PR / the linked issues).
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.
I prefer make_coffeine_data_frame
to be consistent with the function compute_coffeine
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.
well compute_coffeine
is of course also up for discussion but I feel good about it actually.
I can live with make_coffeine_data_frame
and make_covariance_data_frame
– although I have slight preference for coffeine as what we support here is not necessarily a covariance but any SPD matrix.
I would go with make_covariance_data_frame or make_filter_bank_covariance
|
Now would be a good moment for a first round of reviews at the conceptual/strategic/API level. Once the roadmap / idea is clear I‘ll work on new unit tests. |
Also note - I put some thoughts into this with possible future developments in mind (alternative frequency schemes) and alternative feature computation options, including additional covariance estimators (for which we should best update the MNE-Python API, estimator by estimator, as this is easy, specific and gets us all benefits of MNE‘s capability of handling multiple sensor/electrode types + data rank). |
ok seems like we‘ve got docs 😸 |
… Message ID: ***@***.***>
|
works for me
… Message ID: ***@***.***>
|
ok - we've never been that far!
it feels quite big now. unless anyone protests I'd merge this to see how docs / website look. |
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.
you should not commit anything from the generated folder. Please remove it and add it to .gitignore
... one more test missing :) |
Ok, I'm merging 🚀 🎸 – if you have complaints please open an issue / PR 😸 🐙 |
Closes #41 #42 #11 #28 – API included in both notebook and in library.
This PR implements 3 main ideas:
All this should make it far easier for us to express & perform data analyses, reducing biolerplate & cognitive burden. And newcomers should be happier as well.