Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Reduce duplication 1 #1050

Merged
merged 14 commits into from
Aug 2, 2024
Merged

Reduce duplication 1 #1050

merged 14 commits into from
Aug 2, 2024

Conversation

CBroz1
Copy link
Member

@CBroz1 CBroz1 commented Jul 31, 2024

Description

This is a tidying PR that aims to (a) merge existing fully duplicated functions, and (b) increase similarity across functions that will be merged in future PRs. The latter is included as an initial step to make sure what I see as similar is intended as such. This includes cases where the newer version of something was improved without giving the same stylistic edits or additional functionality to the old version.

This partially addresses #977, per checklist in most recent comment.

  • common_ephys.py, B: add spacing/line breaks to reflect updated version
  • common_position.py, B: Add default value for unused variable.
  • decoding/utils.py, A: Common location for _get_peak_amplitude. Updated V0 to pass the waveform extractor instead of waveform
  • decoding/v0/
    • A: Migrate funcs to V0 utils
      • discretize_and_trim
      • get_time_bins_from_interval
      • make_default_decoding_params - required some additional control-flow for cpu/gpu and clusterless/spikes.
    • B: Add comments/spacing to get_X_data
  • decoding/v1/
    • clusterless, B: add track_graph and time_slice params/functionality
    • migrate out subfunc for get_orientation_name
  • linearization/v1/main, B: make table definition comments/spacing like V0 def
  • position/v1/dlc_utils, A: import funcs from common
  • ripple/v1/ripple, A: import func from prev version
  • spikesorting_merge, B: add smoothing sigma param with default
  • other Bs: Adjust to look like match
    • spikesorting_curation
    • spikesorting/v1/curation
    • spikesorting/v1/recording
  • spikesorting/utils.py, A: Extracting core logic of set_group_by_shank into a function that returns insert candidates for parent functions to run. Internal logic otherwise the same
  • spikesorting/v0/curation_figurl, B: Updated to reflect V1 version that uses comprehension over for loops
  • spikesorting/vX/artifact, B: Reflow of docstring, migrate variable reassignment to within ChunkRecordingExecutor call.

Other changes:

  • decoding_merge: removed unused import
  • decoding/v1/core: remove reference to undefined variable in error message
  • See comments about...
    • linearization: potential removal of get_networkx methods
    • spikesorting_merge: lack of a check for ndim==1 compared to alternate version

Checklist:

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

@CBroz1 CBroz1 marked this pull request as ready for review August 1, 2024 01:05
src/spyglass/decoding/v0/clusterless.py Show resolved Hide resolved
src/spyglass/decoding/v0/utils.py Outdated Show resolved Hide resolved
src/spyglass/decoding/v0/utils.py Outdated Show resolved Hide resolved
src/spyglass/decoding/v0/visualization_1D_view.py Outdated Show resolved Hide resolved
src/spyglass/decoding/v0/visualization_2D_view.py Outdated Show resolved Hide resolved
src/spyglass/position/v1/dlc_utils.py Outdated Show resolved Hide resolved
src/spyglass/spikesorting/spikesorting_merge.py Outdated Show resolved Hide resolved
@edeno edeno merged commit 68e8de3 into LorenFrankLab:master Aug 2, 2024
7 checks passed
@CBroz1 CBroz1 deleted the dpe branch August 2, 2024 15:34
@CBroz1 CBroz1 mentioned this pull request Aug 5, 2024
7 tasks
@CBroz1 CBroz1 mentioned this pull request Sep 9, 2024
7 tasks
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.

3 participants