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

Comments on the docsite #86

Closed
TomDonoghue opened this issue Jul 31, 2022 · 6 comments · Fixed by #87
Closed

Comments on the docsite #86

TomDonoghue opened this issue Jul 31, 2022 · 6 comments · Fixed by #87

Comments

@TomDonoghue
Copy link
Contributor

TomDonoghue commented Jul 31, 2022

Collecting a few small / quick comments on the documentation website.
This comment / review is part of the JOSS paper review:
openjournals/joss-reviews#4484

General:

  • In the About part of the repo page (top right), it would be useful to put the documentation site in the link field
  • There seems to be small issue with the version selector (multiple "v0.1"s)
    • Screen Shot 2022-07-31 at 2 23 57 PM
  • The License button in the header is a broken link
  • I think the landing page of the docsite should say a bit more about this project, including a bit more description (such as is in the README / summary of the JOSS paper), and a note that this is a re-implementation of the ICLabel project.

Example:

  • There are some parts that are not rendering correctly (as in, what should be text markdown getting rendered as a code block)
    • This includes the section "# Example: EOG and ECG artifact repair", for both 0.2 release, and the dev version
  • If installing mne-icalabel, with it's required dependencies, and trying to run the example, one hits a dependency error for fitting the ICA (sklearn)
    • I think it might be worth mentioning in the example that it has an extra dependency: scikit-learn
  • For the cell that plots the ICA sources, this is what I get locally:
    • Screen Shot 2022-07-31 at 2 47 53 PM
    • However, one the docsite, it looks like:
    • Screen Shot 2022-07-31 at 2 49 22 PM
    • There appears to be a rendering issue on the docsite, which is an issue for the comments about looking at the time series.
  • After this section of the tutorial, the description and notes discuss "00" as eye activity, and "01" as heartbeat. I agree with "00", but I think something changed at some point such at 01 is not clearly heartbeat, but rather 05 looks more like heartbeat. This is a bit unclear on the docsite, since the time series plot doesn't render, but the time series plot above that gets generated locally seems to suggest this. (Note that the automatic labelling after this also does not label 01 as heartbeat).
  • The Selecting ICA components automatically again has some rendering issues, and I think should be more clearly split up into separate blocks to see each step here
  • There are quite a lot of warnings printed out from the label_components function (local version copied here), and this seems to imply there are some issues with what is being computed? If not, why are the warnings arising?
    • Screen Shot 2022-07-31 at 2 57 07 PM
  • After the labels have been computed, rather than printing them out as a list that is a disconnected from the actual ICs, I think it would be much more informative to then print out
    • something like for ind, label in enumerate(labels): print(label); ica.plot_properties(raw, picks=[ind]);
    • ^It might be a bit much to print all of them, but for a subset showing "this is an IC that the labelling called X" would be nice.
@adam2392
Copy link
Member

@agramfort are you able to provide me with admin access to the repo? I can't change the About field in the repo

@agramfort
Copy link
Member

agramfort commented Aug 1, 2022 via email

@mscheltienne
Copy link
Member

Thank you for the detailed feedback! We will have a look at those items.

@adam2392
Copy link
Member

adam2392 commented Aug 1, 2022

Thanks for the feedback! Will add a comment for each item.

In the About part of the repo page (top right), it would be useful to put the documentation site in the link field

Added!

There seems to be small issue with the version selector (multiple "v0.1"s)
The License button in the header is a broken link

Fixed both license and drop-down menu: https://mne.tools/mne-icalabel/v0.1/index.html#indices-and-tables

@adam2392
Copy link
Member

adam2392 commented Aug 3, 2022

Hi @TomDonoghue I have addressed the remainder of your comments around the example and documentation: https://mne.tools/mne-icalabel/dev/index.html

Thanks so much for the feedback! lmk if you have other issues w/ how we reformatted/re-wrote the example.

@TomDonoghue
Copy link
Contributor Author

This update looks good to me!

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 a pull request may close this issue.

4 participants