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] Revised EEG filters + Store reference fix #9038

Merged
merged 11 commits into from
Feb 29, 2024

Conversation

jeffersoncasimir
Copy link
Contributor

@jeffersoncasimir jeffersoncasimir commented Feb 1, 2024

This PR does the following:

  • Uses a revised set of coefficients values for the filters, depending on the recording's sampling frequency
  • Uses multiple store references when there are multiple recordings. It would previously get overwritten.

The following script was used to generate the coefficients:

samp = 512; % Change to target frequency
order = 3;

low_pass_items = [15 20 30 40 60];
high_pass_items = [0.5 1 5 10];

for i=1:length(low_pass_items)
        disp(strcat(num2str(low_pass_items(i)), ' low pass'));
        [b, a] = butter(order-1, (low_pass_items(i) / samp), 'low') % argument is (order - 1)
end

for i=1:length(high_pass_items)
        disp(strcat(num2str(high_pass_items(i)), ' high pass'));
        [b, a] = butter(order-1, (high_pass_items(i) / samp), 'high') % argument is (order - 1)
end

@christinerogers christinerogers added Bug PR or issue introducing/requiring bug fixes (not mutually exclusive with the Feature label) Critical to release PR or issue is key for the release to which it has been assigned Priority: High PR or issue should be prioritised over others for review and testing labels Feb 6, 2024
@driusan
Copy link
Collaborator

driusan commented Feb 6, 2024

@laemtl can you review?

@christinerogers christinerogers added this to the 26.0.0 milestone Feb 23, 2024
@christinerogers christinerogers added the Add to Release Notes PR change should be highlighted in Release notes (important security, features and bugfixes) label Feb 23, 2024
@laemtl laemtl assigned driusan and unassigned laemtl Feb 27, 2024
@driusan
Copy link
Collaborator

driusan commented Feb 27, 2024

@jeffersoncasimir I can't merge this because of a conflict, can you rebase?

>
<Montage3D />
</ResponsiveViewer>
:
<ResponsiveViewer>
<ResponsiveViewer
// @ts-ignore
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this being ignored? If ts flags something it's usually for a reason.

Copy link
Contributor Author

@jeffersoncasimir jeffersoncasimir Feb 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure why. It was not introduced by this PR (or myself) and there seems to be many instances of // @ts-ignore throughout the module

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh? This line is being introduced by you in this PR.

Copy link
Contributor Author

@jeffersoncasimir jeffersoncasimir Feb 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. However it is also present 11 lines above, as well as in the other <ResponsiveViewer> component in SeriesRenderer.tsx.

It seems to be related to export default withParentSize(ResponsiveViewer) at the end of ResponsiveViewer.tsx.

Here is the error I get when I remove the // @ts-ignore line you pointed out (different branch, but refers to same place):
Screenshot 2024-02-27 at 3 17 54 PM

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds like a real error that needs to be investigated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe I found an appropriate solution. It has been pushed to this PR.

@driusan driusan merged commit 7131f09 into aces:main Feb 29, 2024
28 checks passed
jeffersoncasimir added a commit to jeffersoncasimir/Loris that referenced this pull request Feb 29, 2024
…aces#9038)

This does the following:

- Uses a revised set of coefficients values for the filters, depending on the recording's sampling frequency
- Uses multiple store references when there are multiple recordings. It would previously get overwritten.

The following script was used to generate the coefficients:

```
samp = 512; % Change to target frequency
order = 3;

low_pass_items = [15 20 30 40 60];
high_pass_items = [0.5 1 5 10];

for i=1:length(low_pass_items)
        disp(strcat(num2str(low_pass_items(i)), ' low pass'));
        [b, a] = butter(order-1, (low_pass_items(i) / samp), 'low') % argument is (order - 1)
end

for i=1:length(high_pass_items)
        disp(strcat(num2str(high_pass_items(i)), ' high pass'));
        [b, a] = butter(order-1, (high_pass_items(i) / samp), 'high') % argument is (order - 1)
end
```
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Add to Release Notes PR change should be highlighted in Release notes (important security, features and bugfixes) Bug PR or issue introducing/requiring bug fixes (not mutually exclusive with the Feature label) Critical to release PR or issue is key for the release to which it has been assigned Priority: High PR or issue should be prioritised over others for review and testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants