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

[ENH] Moredocs and experiment with api implementation #31

Merged
merged 9 commits into from
May 2, 2022
Merged

Conversation

adam2392
Copy link
Member

@adam2392 adam2392 commented May 1, 2022

PR Description

Addresses #5 and #19

Merge checklist

Maintainer, please confirm the following before merging:

  • All comments resolved
  • This is not your own PR
  • All CIs are happy
  • PR title starts with [MRG]
  • whats_new.rst is updated
  • PR description includes phrase "closes <#issue-number>"

@codecov
Copy link

codecov bot commented May 1, 2022

Codecov Report

Merging #31 (f83f0db) into main (9a30e79) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #31      +/-   ##
==========================================
+ Coverage   97.44%   97.48%   +0.04%     
==========================================
  Files           8        9       +1     
  Lines         430      437       +7     
==========================================
+ Hits          419      426       +7     
  Misses         11       11              
Impacted Files Coverage Δ
mne_icalabel/iclabel/network.py 100.00% <ø> (ø)
mne_icalabel/iclabel/__init__.py 100.00% <100.00%> (ø)
mne_icalabel/iclabel/config.py 100.00% <100.00%> (ø)
mne_icalabel/iclabel/label_components.py 100.00% <100.00%> (ø)
mne_icalabel/label_components.py 100.00% <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 9a30e79...f83f0db. Read the comment docs.

@adam2392 adam2392 changed the title [ENH] Moredocs and experiment with Transformer api implementation [ENH] Moredocs and experiment with api implementation May 2, 2022
@adam2392 adam2392 requested a review from mscheltienne May 2, 2022 16:18
Copy link
Member

@mscheltienne mscheltienne left a comment

Choose a reason for hiding this comment

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

One comment, otherwise LGTM

Comment on lines +64 to +69
component_dict = {
"y_pred_proba": labels_pred_proba,
"y_pred": labels_pred,
"labels": labels,
}
return component_dict
Copy link
Member

Choose a reason for hiding this comment

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

Any reason for nesting the output in a dictionary? Can't we directly have return y_pred_proba, y_pred, labels (in this order or another)?

Copy link
Member Author

Choose a reason for hiding this comment

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

No real preference.

I only did this to make it super explicit and limit the number of return variables / make it a one-liner to convert to a Dataframe. Would you prefer the 3 tuple return instead?

Copy link
Member

Choose a reason for hiding this comment

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

No strong preference either, but you can't convert this to a DataFrame that easily either since labels_pred_proba is a 2D array.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah fair. Hmm now that I think about it more I do prefer in general single return arguments, but I'm also unsure if this is optimal here... I'll leave this open for now and we can always change it.

Copy link
Member

Choose a reason for hiding this comment

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

I like the idea of simple conversion to DataFrame for the user (of course not on our end, let's not have pandas as a dependency just for that). With that in mind, I see 2 individual DataFrame in the proposed output:

idx labels y_pred
0 Eye 0.89
1 Brain 0.67
... ... ...

and:

idx Brain Muscle Eye Heart Line Noise Channel Noise Other
0 0.2 0.89 0.1 0.05 0.03 0.07 0.08
1 0.03 0.67 0.06 0.04 0.1 0.3 0.09
... ... ... ... ... ... ... ...
n_comp 0.3 0.23 0.43 0.22 0.11 0.08 0.7

So how about 2 outputs: return dict(labels=labels, y_pred=labels_pred), {value: labels_pred_proba[:, k] for k, value in ICLABEL_NUMERICAL_TO_STRING.items()}.

IMO, for what needs to be stored in the mne ICA instance, we do not need the y_pred, but only the labels and the second dict.

With this solution, you can get the 'raw' numpy array of probabilities with mne_icalabel.iclabel.iclabel_label_compoments or dictionaries ready to be transformed to DataFrames with mne_icalabel.label_components.

WDYT?

@adam2392
Copy link
Member Author

adam2392 commented May 2, 2022

I will merge this once the CI complete.

@adam2392 adam2392 merged commit 0a7939a into main May 2, 2022
@adam2392 adam2392 deleted the moredocs branch May 2, 2022 22:35
@adam2392 adam2392 mentioned this pull request May 3, 2022
6 tasks
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.

2 participants