Skip to content

Conversation

@LukeTheHecker
Copy link
Contributor

@LukeTheHecker LukeTheHecker commented Dec 20, 2022

In your list of MNE-friendly packages I replaced my former package ESINet with my updated package "invertmeeg" which implements various EEG inverse solutions. ESINet is replaced since all features are now available in the new package as well.

The package utilizes many mne-python objects like Evoked, Raw, Epochs, Forward, SourceEstimate etc., wherefore I think my package may be of interest to the community.

Keep up the good work!
-Lukas

In your list of MNE-friendly packages I replaced my former package ESINet with my updated package "invertmeeg" which implements various EEG inverse solutions. ESINet is replaced since all features are now available in the new package as well.

The package utilizes many mne-python objects like Evoked, Raw, Epochs, Forward, SourceEstimate etc., wherefore I think my package may be of interest to the community.

Keep up the good work!
-Lukas
@agramfort
Copy link
Member

@LukeTheHecker did you consider contributing some of these solvers in mne directly?

I see a number of issues in your repo with build files that should not be in the repo.

Do you have an example gallery that runs each solver on the same dataset to see how it looks?

Your testing is quiet crude (just smoke tests). How did you assess that your code is correct? See how unit tests are done for solvers in mne here

@LukeTheHecker
Copy link
Contributor Author

Thank you @agramfort for taking time to review the package!

@LukeTheHecker did you consider contributing some of these solvers in mne directly?

I have thought about it, surely this would be the optimal solution for the users. I assume that it would take quite some time to adapt the code architecture to fit mne-python. What do you think?

I see a number of issues in your repo with build files that should not be in the repo.

Huh, I didn't know that this would be an issue. Do you mean the build files for the pypi-packaging (./build/) ?

Do you have an example gallery that runs each solver on the same dataset to see how it looks?

Yes, I will add a gallery to the documentation in the future

Your testing is quiet crude (just smoke tests). How did you assess that your code is correct? See how unit tests are done for solvers in mne here

Thanks for the suggestion! In deed, they are just smoke tests at the moment and I plan to add unit tests in the future.

@larsoner larsoner merged commit 654248a into mne-tools:main Dec 20, 2022
@larsoner
Copy link
Member

Thanks for the fixed link @LukeTheHecker ! Feel free to continue discussing with @agramfort here though :)

@LukeTheHecker
Copy link
Contributor Author

@larsoner thanks for adding the package :-)

@agramfort
Copy link
Member

@LukeTheHecker did you consider contributing some of these solvers in mne directly?

I have thought about it, surely this would be the optimal solution for the users. I assume that it would take quite some time to adapt the code architecture to fit mne-python. What do you think?

I assume you use some MNE solvers in your package right? Basically you have an object oriented API and then internal plain functions no? At least that doable no?

I see a number of issues in your repo with build files that should not be in the repo.

Huh, I didn't know that this would be an issue. Do you mean the build files for the pypi-packaging (./build/) ?

rule of thumb is that your repo should not contain auto generated files

Do you have an example gallery that runs each solver on the same dataset to see how it looks?

Yes, I will add a gallery to the documentation in the future

that would be great

Your testing is quiet crude (just smoke tests). How did you assess that your code is correct? See how unit tests are done for solvers in mne here

Thanks for the suggestion! In deed, they are just smoke tests at the moment and I plan to add unit tests in the future.

great

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.

3 participants