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

Repo reorganization #29

Open
mesca opened this issue Apr 21, 2020 · 6 comments
Open

Repo reorganization #29

mesca opened this issue Apr 21, 2020 · 6 comments

Comments

@mesca
Copy link
Member

mesca commented Apr 21, 2020

At this point, we should clean and reorganize this repo before publishing it. We have a couple options: either we start a new repo from the timeflux_example template and update it, or we transfer this one to the timeflux organization. I don't have any preference, but if we start from a clean slate, we must be sure to create at least one new issue to reference the current problems and report the backlog for future reference.

For consistency, I would recommend the following structure:

timeflux_rasr
    doc
        *.rst
    test
        test_*.py
    examples
        *.yaml
    notebooks
        *.ipynb
    timeflux_rasr
        estimators
            rasr.py
            blending.py
        helpers
            *.py
    .gitignore
    LICENSE
    README.md
    requirements.txt
    setup.py

I will update the examples directory with at least one usage example and a small annotated data file.

estimation.py should be renamed to rasr.py.

In the helpers directory, please keep only what you actually use from utils.py and maybe rename the file to something more meaningful. We will later decide with @bertrandlalo what could be migrated to Timeflux core.

Please make sure that the dependencies in requirements.txt and setup.py are satisfied. The file should contain the absolute minimum to make it run in a Timeflux pipeline. If other dependencies are needed for the notebooks (mne, matplotlib, etc.), write another dependency file.

I am not sure to understand what purpose test_config.py and test_viz actually serve.

The hardcoded paths in utils/config.py made me cry ;)

@jonathanjfshaw
Copy link

Another Python RASR implementation just got published today: https://github.com/nbara/python-meegkit

@lkorczowski
Copy link
Collaborator

@jonathanjfshaw : thanks. There are some significant difference in design from the Matlab implementation here, so both version could be useful.

@sylvchev : I think they struggling also with pymanopt because the nonlinear_eigenspace is implemented but not in use.

@nbara
Copy link

nbara commented Nov 4, 2020

Hi @lkorczowski , I wrote the other ASR python code above. Let me know if I can be of any help to compare/benchmark/test our respective implementations. The Matlab code is anything but straightforward, so the most important thing is that we get the expected results.

@lkorczowski
Copy link
Collaborator

@nbara : Great. Thanks.
So far, the #30 has already a visualization (qualitative) framework for comparison with the matlab output, it should be straightforward to compare with your implementation.

We still had some trouble finding the right metric to make a quantitative comparison. IMO, the best way would be to use hybrid EEG data (clean + artificial artifacts) but it will be bit of work to make implement this framework.

Do you have more ideas?

@nbara
Copy link

nbara commented Nov 9, 2020

If you can already check that your implementation gives the same result as Matlab's visually, I think you're fine.

I agree that some kind of quantitative test would be even better (because the original Matlab code could contain bugs as well after all).

You may already have read it but maybe this paper will give you some ideas?
https://ieeexplore.ieee.org/abstract/document/8768041

@mesca
Copy link
Member Author

mesca commented Jun 6, 2021

@lkorczowski

The repo is nearly ready for publication. More cleanup is necessary, but it can wait after we have a PIP package:

  • remove unnecessary files/methods in helpers
  • remove private paths in helpers
  • fix import paths in notebooks
  • give instructions on how to get the required data to run the notebooks

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

No branches or pull requests

4 participants