timeflux / timeflux_rasr

Implementation of rASR filtering.
MIT License
26 stars 4 forks source link

[ROADMAP] Sprint3 #15

Closed lkorczowski closed 3 years ago

lkorczowski commented 4 years ago

SPRINT BACKLOG

in chronological order

PRODUCT BACKLOG (out of scope)

mesca commented 4 years ago

It is very important that the library can be used by non-specialist users. That means:

mesca commented 4 years ago

Tests must be extensive, and must address every possible breaking point in the code. It is usually a bad practice to do them at the end. Have they been done on the fly, some issues raised in #17 would have been detected earlier.

mesca commented 4 years ago

I would add:

We will acquire the data along with a synchronized video feed, and annotate classic artifacts ourselves.

lkorczowski commented 4 years ago

It is very important that the library can be used by non-specialist users. That means:

* Clearly document the input parameters

* Automatically infer secondary parameter values when possible

* Provide sensible defaults, with the number of required parameters kept to the minimum
lkorczowski commented 4 years ago

Tests must be extensive, and must address every possible breaking point in the code. It is usually a bad practice to do them at the end. Have they been done on the fly, some issues raised in #17 would have been detected earlier.

I understand, this procedure is new for me but I'm learning. I'll work on that until your expectations are met.

lkorczowski commented 4 years ago

In #21, @bertrandlalo mentioned that passing parameters from RASR to _fit_eeg_distribution() may use **kwargs. However, I couldn't find the best pratice to avoid copypasta of docstrings and avoid the initialization of the parameters in RASR().

I'm adding this problem in the backlog. I don't think it is critical right now. I simply can't figure out myself what is the best practice here.

lkorczowski commented 4 years ago

[ ] Visually validate on a new dataset

Added to backlog

lkorczowski commented 4 years ago

following https://github.com/bertrandlalo/timeflux_rasr/issues/5#issuecomment-584352525 , I added the code refactoring to remove 2D matrix compatibility.

lkorczowski commented 4 years ago

following https://github.com/bertrandlalo/timeflux_rasr/issues/5#issuecomment-584352525 (removal of 2D compatibility) , I propose to develop a specific node for blending outside of RASR(). Rational discussed in https://github.com/bertrandlalo/timeflux_rasr/issues/4#issuecomment-584582494

lkorczowski commented 4 years ago

Following https://github.com/bertrandlalo/timeflux_rasr/pull/21#issuecomment-585227534 , I am adding the task to pass the parameters from RASR to _fit_eeg_distribution using **kwargs and:

lkorczowski commented 4 years ago

Sprint3 starts now.

lkorczowski commented 4 years ago

FYI 1bf6683 added a feature for testing and passing **kwargs arguments properly while still checking for invalids key parameters. feature done

lkorczowski commented 4 years ago

@mesca could you send me the new dataset ? Thanks

lkorczowski commented 4 years ago

@mesca in news on the #27 review ?

lkorczowski commented 3 years ago

27 and #30 fixed most of the points

34 tested on new dataset