timeflux_rasr icon indicating copy to clipboard operation
timeflux_rasr copied to clipboard

Repo reorganization

Open mesca opened this issue 5 years ago • 6 comments

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 ;)

mesca avatar Apr 21 '20 20:04 mesca

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

jonathanjfshaw avatar May 01 '20 12:05 jonathanjfshaw

@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.

lkorczowski avatar Jun 16 '20 09:06 lkorczowski

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.

nbara avatar Nov 04 '20 09:11 nbara

@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?

lkorczowski avatar Nov 04 '20 11:11 lkorczowski

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

nbara avatar Nov 09 '20 12:11 nbara

@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

mesca avatar Jun 06 '21 16:06 mesca