Skip to content

Conversation

@MingzeDou
Copy link
Contributor

No description provided.

Copilot AI review requested due to automatic review settings December 18, 2025 13:05
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds a new power calculation feature to NeuroScope2, allowing users to compute and visualize spectral power across channels for a specified time window and frequency band. The implementation includes interactive dual-plot visualization (spatial map and channel-ordered line plot) with click-to-identify functionality.

Key Changes:

  • Added "Power Calculation" menu item under the Analysis menu
  • Implemented calculatePower function with frequency band filtering and power computation
  • Added powerPlotClickCallback for interactive plot navigation
  • Updated UI panel minimum height to accommodate new functionality
  • Various minor formatting fixes (trailing whitespace removal)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

try
% fseek to position
% Position in bytes: sample_index * nChannels * bytes_per_sample (2 for int16)
offset_bytes = start_sample * nChannels * 2;
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

The hardcoded value '2' for bytes per sample assumes int16 precision. However, the precision is read from UI.settings.precision which could potentially be different (e.g., 'int32', 'single', 'double'). This could cause incorrect file positioning and data corruption if the actual data type differs from int16.

Copilot uses AI. Check for mistakes.
Comment on lines +4642 to +4652
if isnan(t_start) || isnan(t_end) || t_end <= t_start
MsgLog('Invalid Time Range.', 4);
return;
end

if isnan(low_freq) || isnan(high_freq) || high_freq <= low_freq
MsgLog('Invalid Frequency Range.', 4);
return;
end

duration = t_end - t_start;
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

No validation that t_start is non-negative or that t_end doesn't exceed the recording duration. Users could request data beyond file boundaries, causing fseek/fread errors. Add bounds checking against the total recording time available.

Copilot uses AI. Check for mistakes.

% Hidden/Unused controls for compatibility if needed, or remove.
% Keeping them valid but hidden is safer if logic depends on them, but we hardcode 4-12Hz now.
% So strictly speaking we don't need them in UI, but to avoid errors if I miss any reference, I will removing them from UI creation.
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

The comment states "I will removing them from UI creation" which is grammatically incorrect and unclear. Should be "I will remove them from UI creation" or "I am removing them from UI creation".

Suggested change
% So strictly speaking we don't need them in UI, but to avoid errors if I miss any reference, I will removing them from UI creation.
% So strictly speaking we don't need them in UI, but to avoid errors if I miss any reference, I will remove them from UI creation.

Copilot uses AI. Check for mistakes.
Comment on lines +4694 to +4704
try
% fseek to position
% Position in bytes: sample_index * nChannels * bytes_per_sample (2 for int16)
offset_bytes = start_sample * nChannels * 2;
fseek(curr_fid, offset_bytes, 'bof');

% fread
raw_data = fread(curr_fid, [nChannels, n_samples], precision);

% Transpose to (samples x channels) and scale
traces = double(raw_data') * UI.settings.leastSignificantBit;
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

Loading potentially large amounts of data (duration * sampling rate * channels) into memory without size validation. For long durations or high sampling rates, this could cause memory exhaustion. Consider adding a check for maximum data size or implementing chunked processing for large requests.

Copilot uses AI. Check for mistakes.
[b, a] = butter(2, [low_freq, high_freq] / nyquist, 'bandpass');
end

filtered_traces = filtfilt(b, a, traces);
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

The filtfilt operation is applied to all channels simultaneously without try-catch protection. If the filter becomes unstable during execution (despite the stability check), this will crash the function. Wrap filtfilt in error handling to provide graceful failure.

Suggested change
filtered_traces = filtfilt(b, a, traces);
try
filtered_traces = filtfilt(b, a, traces);
catch ME
MsgLog(sprintf('Bandpass filtering failed (%s). Using unfiltered traces instead.', ME.message), 3);
filtered_traces = traces;
end

Copilot uses AI. Check for mistakes.
% Defining flexible panel heights
set(UI.panel.analysis.main, 'Heights', [150 60 100 100 100],'MinimumHeights',[150 60 100 100 100]);
UI.panel.analysis.main1.MinimumWidths = 218;
UI.panel.analysis.main1.MinimumHeights = 510;
UI.panel.analysis.main1.MinimumHeights = 710;
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

The MinimumHeights value changed from 510 to 710, presumably to accommodate the new Power Calculation menu item. However, this is a significant UI change (200 pixel increase) that may affect the layout on smaller screens or cause UI overlap. Verify that this works correctly across different screen resolutions and consider making this more flexible.

Suggested change
UI.panel.analysis.main1.MinimumHeights = 710;
% Set analysis panel minimum height adaptively to work across screen sizes
screenSize = get(0,'ScreenSize'); % [left bottom width height]
screenHeight = screenSize(4);
desiredMinHeight = 710; % height to accommodate new menu item
baseMinHeight = 510; % previous minimum height (fallback)
maxAllowedHeight = max(baseMinHeight, screenHeight - 150); % keep some vertical margin
minAnalysisHeight = min(desiredMinHeight, maxAllowedHeight);
UI.panel.analysis.main1.MinimumHeights = minAnalysisHeight;

Copilot uses AI. Check for mistakes.
Comment on lines +4735 to +4738
end

filtered_traces = filtfilt(b, a, traces);

Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

Filter stability check using roots(a) may fail for edge cases or produce numerical instability warnings. After reducing to order 2, there's no verification that the second-order filter is stable, and no fallback if it also fails. Consider adding a stability check after order reduction or using filtfilt's error handling.

Suggested change
end
filtered_traces = filtfilt(b, a, traces);
% Re-check stability after order reduction
if any(abs(roots(a)) >= 1)
MsgLog('Second-order filter also unstable. Skipping filtering and using raw traces.', 4);
filtered_traces = traces;
else
try
filtered_traces = filtfilt(b, a, traces);
catch ME
MsgLog(['Filtering failed for second-order filter: ' ME.message], 4);
filtered_traces = traces;
end
end
else
% Original 3rd-order filter is stable
try
filtered_traces = filtfilt(b, a, traces);
catch ME
MsgLog(['Filtering failed for third-order filter: ' ME.message], 4);
filtered_traces = traces;
end
end

Copilot uses AI. Check for mistakes.

% Bandpass Filter
% Downgraded to 3rd order Butterworth to improve stability for narrow/low bands
nyquist = fs / 2;
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

The filter normalization check (high_freq / nyquist) should ensure that the normalized frequencies are less than 1.0 to avoid filter design errors. Add validation to check that high_freq is less than the Nyquist frequency (fs/2) before filter design.

Suggested change
nyquist = fs / 2;
nyquist = fs / 2;
% Ensure band limits are valid relative to Nyquist to avoid filter design errors
if high_freq >= nyquist
adjHigh = nyquist - eps(nyquist);
MsgLog(sprintf('High cutoff frequency (%.4f Hz) >= Nyquist (%.4f Hz). Adjusting to %.4f Hz.', ...
high_freq, nyquist, adjHigh), 3);
high_freq = adjHigh;
end
if low_freq < 0
MsgLog(sprintf('Low cutoff frequency (%.4f Hz) < 0 Hz. Adjusting to 0 Hz.', low_freq), 3);
low_freq = 0;
end
if low_freq >= high_freq
% Ensure a valid band; nudge low_freq below high_freq if needed
newLow = max(0, high_freq * 0.5);
MsgLog(sprintf('Low cutoff frequency (%.4f Hz) >= high cutoff (%.4f Hz). Adjusting low cutoff to %.4f Hz.', ...
low_freq, high_freq, newLow), 3);
low_freq = newLow;
end

Copilot uses AI. Check for mistakes.
Comment on lines +698 to +704
% 4-12Hz Power Heatmap
% Power Calculation

% Hidden/Unused controls for compatibility if needed, or remove.
% Keeping them valid but hidden is safer if logic depends on them, but we hardcode 4-12Hz now.
% So strictly speaking we don't need them in UI, but to avoid errors if I miss any reference, I will removing them from UI creation.

Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

Incomplete or obsolete comments that don't explain the purpose. These comments mention "4-12Hz Power Heatmap" and "Hidden/Unused controls" but don't provide clear context about what was removed or why. Either complete the explanation or remove these placeholder comments.

Copilot uses AI. Check for mistakes.
set(plotData.hInfo, 'String', txt_str);
end
end

function setSortingMetric(~,~)
Copy link

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

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

The setSortingMetric function body was removed, leaving only the uiresume call. If the function previously set UI.params.sortingMetric (as suggested by the function name), this functionality is now lost. Verify that removing this assignment doesn't break sorting metric selection functionality.

Suggested change
function setSortingMetric(~,~)
function setSortingMetric(~,~)
UI.params.sortingMetric = UI.panel.cell_metrics.sortingMetric.String{UI.panel.cell_metrics.sortingMetric.Value};

Copilot uses AI. Check for mistakes.
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

Successfully merging this pull request may close these issues.

1 participant