Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[electrophysiology_browser] Effect of filters between chunks #8717

Open
jeffersoncasimir opened this issue May 30, 2023 · 10 comments
Open

[electrophysiology_browser] Effect of filters between chunks #8717

jeffersoncasimir opened this issue May 30, 2023 · 10 comments
Labels
Bug PR or issue introducing/requiring bug fixes (not mutually exclusive with the Feature label)

Comments

@jeffersoncasimir
Copy link
Contributor

jeffersoncasimir commented May 30, 2023

Describe the bug
In between chunks, there appears to be a gap, only visible when zoomed in. When applying filters, the gap becomes very visible, as the signals appear to diverge from the gap in a noticeable way. These filters should also be validated to determine if they are appropriate for more than one specific set of recording parameters.

Screenshot 2023-05-30 at 2 57 16 PM
Screenshot 2023-05-30 at 2 57 31 PM

@jeffersoncasimir jeffersoncasimir added the Bug PR or issue introducing/requiring bug fixes (not mutually exclusive with the Feature label) label May 30, 2023
@jeffersoncasimir jeffersoncasimir changed the title [Electrophysiology Broswer] [electrophysiology_broswer] Effect of filters between chunks May 30, 2023
@jeffersoncasimir jeffersoncasimir changed the title [electrophysiology_broswer] Effect of filters between chunks [electrophysiology_browser] Effect of filters between chunks May 30, 2023
@christinerogers
Copy link
Contributor

christinerogers commented May 30, 2023

@jeffersoncasimir to take an hour before Friday 11 am and see if the solution is easy, consult with @laemtl if needed.

@jeffersoncasimir
Copy link
Contributor Author

jeffersoncasimir commented Jun 2, 2023

The cause for the gap is identified and is due to an off-by-one error.

This line should be inclusively producing all equally distributed numbers within the interval, but never equals interval[1] since i / values.length is never 1.

The unexpected filter behaviour between chunks is caused by filters being applied to chunks discretely as opposed to interpreting all visible points as a continuous signal.

@christinerogers
Copy link
Contributor

est 2-4 weeks to refactor the filter application ->

could interpolation solve it ?

LF and JC to touch base

@christinerogers
Copy link
Contributor

christinerogers commented Jun 12, 2023

is this is beginner friendly issue @jeffersoncasimir ?

@jeffersoncasimir
Copy link
Contributor Author

It is not

@christinerogers
Copy link
Contributor

These filters should also be validated to determine if they are appropriate for more than one specific set of recording parameters.
This is because they were hard-coded in the original Open MNI Atlas (for that recording, w values from N.von Ellenrieder) and then brought into LORIS. Not relevant to all recordings.

@christinerogers christinerogers added this to the 25.0.0 milestone Sep 22, 2023
@christinerogers
Copy link
Contributor

christinerogers commented Sep 22, 2023

un/related: #8892 will be in the 25.1 release (ideally), but not too related to this issue which will persist

@christinerogers
Copy link
Contributor

christinerogers commented Sep 22, 2023

break this down, seems we have the following options (consult w LF and bring possible plans to the Loris-EEG meeting when ready)

  1. remove all filters
  2. @jefferson what's a small/medium scope solution? e.g. limit filter options, pre-cache...
  3. solve with dynamic calculations across chunk boundaries in the browser - what's the perfomance

@christinerogers christinerogers removed this from the 25.0.0 milestone Sep 22, 2023
@jeffersoncasimir
Copy link
Contributor Author

jeffersoncasimir commented Oct 20, 2023

A solution has been implemented, with a PR coming soon, which performs the signal filter on all visible chunks, instead of chunks individually. This is achieved by concatenating them all into one chunk/signal and works similarly to how DC offset was implemented.

This has a performance cost by introducing technically redundant memoizations but there is a lot of room for improvement on that front, which is due for a revisit.

@cmadjar
Copy link
Collaborator

cmadjar commented Oct 1, 2024

@jeffersoncasimir has a PR been sent and merged in the end? Should this be closed?

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Bug PR or issue introducing/requiring bug fixes (not mutually exclusive with the Feature label)
Projects
None yet
Development

No branches or pull requests

3 participants