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

Notes on JOSS paper #83

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

Notes on JOSS paper #83

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

Comments

@TomDonoghue
Copy link
Contributor

Some brief comments / suggestions on the current draft of the JOSS paper:

  • lines 16-18: I find this sentence a little vague, and a couple sentences could clarify the details
    • how it makes this available: "this work makes the popular Matlab-based ICLabel model available in MNE-Python
    • something more on what "modern Pytorch format", as this is a bit unclear
  • line 41: say "we will build" but since this project now exists, the tense shouldn't be future
  • the relationship between Pytorch / Tensorflow, and what this project does related to them is unclear. For the M/EEG researcher who's looking into this project, I think it may be unclear how / why you jump between these different projects without detailing how they relate to each other and a more detailed description of the status of the old tool, and the update here.

Note: this issue is part of the JOSS review:
openjournals/joss-reviews#4484

@adam2392
Copy link
Member

adam2392 commented Jul 31, 2022

Some brief comments / suggestions on the current draft of the JOSS paper:

  • lines 16-18: I find this sentence a little vague, and a couple sentences could clarify the details

    • how it makes this available: "this work makes the popular Matlab-based ICLabel model available in MNE-Python
    • something more on what "modern Pytorch format", as this is a bit unclear

Thanks for the comment, we've changed the sentence from: This work adds 17 the popular ICLabel model (Pion-Tonachini et al., 2019) to the MNE-Python (Gramfort et al., 18 2013) software toolkit in a modern Pytorch format (Paszke et al., 2019). to now say the following:

This work makes the popular ICLabel model [@iclabel2019] available in Python by creating a software package compatible with MNE-Python [@Agramfort2013] software toolkit and a modern Pytorch format [@Pytorch2019]. The ICLabel model was previously only available in an outdated version of tensorflow that was no longer supported, and migrating the model now to an updated Pytorch version will ensure the model will not break due to unmaintained versions of software.

  • line 41: say "we will build" but since this project now exists, the tense shouldn't be future

Here, we meant to specify our future plans for the repository, which would extend beyond just the ICLabel model. I have modified the sentence to state:

In future versions, we plan on building on top of the ICLabel model to improve its robustness when operating on different types of recording hardware, sensor type, and sensor count.

@adam2392
Copy link
Member

  • the relationship between Pytorch / Tensorflow, and what this project does related to them is unclear. For the M/EEG researcher who's looking into this project, I think it may be unclear how / why you jump between these different projects without detailing how they relate to each other and a more detailed description of the status of the old tool, and the update here.

I added the following sentences in the Statement of Need.

Pytorch is the most common Python deep-learning framework among researchers. Replicating the model in Pytorch enables future automatic ICA annotation research to easily build upon the model.

Does this address this concern?

@jacobf18
Copy link
Collaborator

@adam2392 Small fix, but PyTorch is spelled with camel case.

@adam2392 adam2392 reopened this Aug 3, 2022
@adam2392 adam2392 closed this as completed Aug 3, 2022
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.

3 participants