Skip to content

Conversation

@edeno
Copy link
Collaborator

@edeno edeno commented Apr 15, 2025

Description

  • Adding a condition in the MAD detector to replace zero, NaN, or infinite MAD values with 1.0.
  • Refactoring the creation of LFPElectrodeGroup with added input validation and transactional insertion.
  • Updating the LFPBandSelection logic with comprehensive validation and batch insertion for electrodes and references.
  • Remove ability to decimate LFP data in LFPBandSelection using set_lfp_band_electrodes

Checklist:

  • This PR should be accompanied by a release: (yes/no/unsure)
  • If release, I have updated the CITATION.cff
  • This PR makes edits to table definitions: (yes/no)
  • If table edits, I have included an alter snippet for release notes.
  • If this PR makes changes to position, I ran the relevant tests locally.
  • I have updated the CHANGELOG.md with PR number and description.
  • I have added/edited docs/notebooks to reflect the changes

@edeno edeno requested review from CBroz1 and Copilot April 15, 2025 14:52
Copy link
Contributor

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.

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

@edeno edeno requested a review from Copilot April 15, 2025 14:57
Copy link
Contributor

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.

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

@edeno edeno requested a review from Copilot April 22, 2025 04:06
Copy link
Contributor

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 improves LFP processing by adding atomic insertion of electrode groups and band selection steps, validating input parameters, and enhancing the MAD detector to handle edge cases (0, NaN, and inf values). Key changes include:

  • Adding a condition in the MAD detector to replace zero, NaN, or infinite MAD values with 1.0.
  • Refactoring the creation of LFPElectrodeGroup with added input validation and transactional insertion.
  • Updating the LFPBandSelection logic with comprehensive validation and batch insertion for electrodes and references.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
src/spyglass/lfp/v1/lfp_artifact_MAD_detection.py Adjusts MAD computation and thresholding logic for robustness.
src/spyglass/lfp/lfp_electrode.py Refines electrode group creation with input validation and proper transaction handling.
src/spyglass/lfp/analysis/v1/lfp_band.py Enhances band selection validation and batch inserts paired electrode-reference entries.
Comments suppressed due to low confidence (1)

src/spyglass/lfp/analysis/v1/lfp_band.py:127

  • [nitpick] Sorting and deduplication via set() may reorder the original electrode order, which could be important for downstream processing. Consider preserving the original order if order is significant or add a comment to clarify that order does not matter.
paired_list = sorted(list(set([(e, common_ref) for e in electrode_list])))

@edeno edeno requested a review from Copilot April 22, 2025 15:49
Copy link
Contributor

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 improves LFP processing by enhancing error handling, input validation, and transactional atomicity in LFP detector and selection routines. Key changes include:

  • Updating the MAD detector to handle 0, NaN, or infinite values.
  • Refining electrode group creation with robust validation and transactional insertion.
  • Modifying the LFP band selection logic for better input handling and clearer error messaging.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
src/spyglass/lfp/v1/lfp_artifact_MAD_detection.py Enhanced MAD computation and threshold logic.
src/spyglass/lfp/lfp_electrode.py Improved input validation and transactional electrode insertion.
src/spyglass/lfp/analysis/v1/lfp_band.py Updated LFP band selection handling and error message clarity.
Comments suppressed due to low confidence (1)

src/spyglass/lfp/analysis/v1/lfp_band.py:200

  • [nitpick] Consider using consistent f-string formatting for the error message here so that master_key is rendered with its proper values.
raise ValueError(

@edeno edeno marked this pull request as ready for review April 22, 2025 15:53
@edeno edeno requested a review from samuelbray32 April 22, 2025 15:53
@edeno edeno linked an issue Apr 22, 2025 that may be closed by this pull request
@edeno edeno added the enhancement New feature or request label Apr 22, 2025
Copy link
Contributor

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 implements improvements to the LFP processing modules by updating the MAD detector to replace zero, NaN, or infinite values, refactoring LFPElectrodeGroup creation with enhanced input validation and transactional insertion, and updating LFPBandSelection logic with comprehensive validation and batch electrode/reference insertion.

  • Update MAD detector logic to handle non-finite or zero values
  • Refactor LFPElectrodeGroup creation with added input validation and transaction management
  • Enhance LFPBandSelection electrode/reference validation and remove decimation approach

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
src/spyglass/lfp/v1/lfp_artifact_MAD_detection.py Improved handling of bad MAD values using numpy conditionals
src/spyglass/lfp/lfp_electrode.py Refactored electrode group creation with better input validation
src/spyglass/lfp/analysis/v1/lfp_band.py Updated electrode/reference validation and insertion logic
CHANGELOG.md Documented LFP improvements
Comments suppressed due to low confidence (1)

src/spyglass/lfp/analysis/v1/lfp_band.py:187

  • Ensure that the ordering of the electrode_group_name_list returned by fetch aligns with the order of electrode_list. A mismatch in ordering could lead to incorrect association of electrode group names during batch insertion.
            & [ {"nwb_file_name": nwb_file_name, "lfp_electrode_group_name": lfp_electrode_group_name, "electrode_id": electrode_id, } for electrode_id in electrode_list ]

@edeno edeno requested a review from Copilot April 23, 2025 13:37
Copy link
Contributor

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 implements several improvements to the LFP processing pipelines including:

  • Updating the MAD detector to replace zero, NaN, or infinite MAD values with 1.0.
  • Refactoring LFPElectrodeGroup creation with enhanced input validation and transactional insertion.
  • Overhauling the LFPBandSelection logic with comprehensive validation and batch insertion for electrodes and references.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
src/spyglass/lfp/v1/lfp_artifact_MAD_detection.py Improved MAD detection logic by handling zero, NaN, or infinite MAD values.
src/spyglass/lfp/lfp_electrode.py Enhanced LFPElectrodeGroup creation with additional input checks and transactional insertion.
src/spyglass/lfp/analysis/v1/lfp_band.py Updated electrode and reference validation along with batch insertion in LFPBandSelection logic.
CHANGELOG.md Documented the LFP improvements in the release notes.

@edeno edeno requested a review from Copilot April 23, 2025 15:54
Copy link
Contributor

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 improves the LFP processing pipelines with enhanced validation, error handling, and transactional operations. Key changes include:

  • Replacing zero, NaN, or infinite MAD values with 1.0 in the MAD detector.
  • Refactoring LFPElectrodeGroup creation with input validation and atomic insertion.
  • Updating LFPBandSelection logic with comprehensive validation, improved electrode/reference handling, and removal of decimation functionality.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
src/spyglass/lfp/v1/lfp_artifact_MAD_detection.py Adjusts MAD detector handling and threshold evaluation logic.
src/spyglass/lfp/lfp_electrode.py Refactors LFPElectrodeGroup creation with enhanced input validation and transaction management.
src/spyglass/lfp/analysis/v1/lfp_band.py Updates electrode and reference validation, signal computation methods, and error handling.
CHANGELOG.md Updates the changelog to include the new enhancements.
Comments suppressed due to low confidence (1)

src/spyglass/lfp/analysis/v1/lfp_band.py:70

  • The parameter documentation specifies 'int or list[int]' but the code also supports numpy arrays. Update the docstring and type annotation to explicitly include numpy array support for clarity.
reference_electrode_list: int or list[int]

@edeno edeno requested a review from CBroz1 April 23, 2025 16:00
@edeno edeno merged commit 46d59d6 into master Apr 24, 2025
20 checks passed
@edeno edeno deleted the lfp-improvements branch April 24, 2025 01:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request LFP

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Integrity issue for LFPBandSelection

4 participants