Skip to content

Conversation

@CBroz1
Copy link
Member

@CBroz1 CBroz1 commented Sep 6, 2024

Description

This PR adds the ability to recompute when fetching a non-existent file from v1.SpikeSortingRecording, adding some infrastructure to support expanding this tool in the future. To facilitate this, I add ...

  • A standard of a _make_file func that other tables can implement in the future.
  • NWB file specific and more general directory hashing tools
  • Tests for round-trip file deletion, then calling fetch_nwb or a new v0 load_recording function that centralizes file regeneration logic

Closes #773
Closes #917
Closes #1272

Checklist:

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

@CBroz1
Copy link
Member Author

CBroz1 commented Sep 16, 2024

I ran in to some issues here, being unable to replicate the DJ-stored file hash with seemingly matched contents. Need to do more testing on whether or not memory pointers are factored into the hash. If so, we may need to store a hash of the object computed by the specific table, and adjust the DJ-stored file hash on recreation

@edeno edeno added the enhancement New feature or request label Sep 25, 2024
@edeno edeno self-requested a review February 20, 2025 17:12
Copy link
Collaborator

@edeno edeno left a comment

Choose a reason for hiding this comment

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

I think the overall structure looks pretty good. For comparing arrays, remind me why something like np.allclose wouldn't work well?

@edeno
Copy link
Collaborator

edeno commented Feb 24, 2025

Just adding a reminder here to warn the lab before this gets merged.

@CBroz1
Copy link
Member Author

CBroz1 commented Apr 11, 2025

... attempts recompute on something that exists on our filesystem but not on theirs?

I've updated the logic to defer recompute until after the check of dandi/kachery. I think it's the case that the recompute is only going to be attempted if we have a database entry, but no corresponding file. Therefore, it would only trigger for a collaborator if they loaded a dump of our database, but didn't get all the files ... and also, the files are on neither remote store

Copy link
Collaborator

@edeno edeno left a comment

Choose a reason for hiding this comment

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

Just some minor things and questions. I am still working on testing out the code on real data.

@edeno edeno requested a review from Copilot May 8, 2025 12:20
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 introduces a new recompute ability via a dedicated _make_file function that enables dynamic regeneration of NWB files when they are missing. Key changes include standardizing file creation across multiple pipelines, updating file‐path and hashing tools in AnalysisNwbfile, and removing redundant logging calls to streamline the recompute process.

Reviewed Changes

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

Show a summary per file
File Description
src/spyglass/lfp/v1/lfp.py Removed a redundant logging call in the make method to support the new recompute approach.
src/spyglass/lfp/analysis/v1/lfp_band.py Similar removal of logging call to standardize behavior across LFP analysis tables.
src/spyglass/decoding/v1/waveform_features.py Removed pre-create timing call and logging call associated with NWB file creation.
src/spyglass/decoding/v0/clusterless.py Removed logging call from file creation logic to simplify recompute handling.
src/spyglass/common/common_nwbfile.py Updated the AnalysisNwbfile.create and get_abs_path methods to accept additional parameters for recompute functionality.
docs/* Updated documentation pages and changelog to reflect the new recompute feature.
pyproject.toml & .pre-commit-config.yaml Updated dependency versions and configuration adjustments.
Comments suppressed due to low confidence (2)

src/spyglass/lfp/v1/lfp.py:201

  • [nitpick] The removal of the logging call appears intentional to support the new recompute workflow; consider adding an inline comment or documentation note explaining the alternative mechanism for tracking NWB file creation events.
AnalysisNwbfile().log(key, table=self.full_table_name)

src/spyglass/decoding/v0/clusterless.py:262

  • [nitpick] As with other parts of the codebase, the removal of the logging call should be accompanied by documentation that explains how NWB file creation events are now tracked, ensuring consistency across the recompute pipelines.
AnalysisNwbfile().log(key, table=self.full_table_name)

@edeno edeno merged commit c0c86ca into LorenFrankLab:master May 8, 2025
17 checks passed
@CBroz1 CBroz1 deleted the rcp branch May 13, 2025 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

3 participants