Standardize nwb ingestion #1377

samuelbray32 · 2025-08-20T19:34:13Z

Description

Resolves #1375, partially resolves #1326

Implements a standard set of functions and properties for tables that ingest from nwb files

Known Tables for Ingestion

Checklist:

N/a. If this PR should be accompanied by a release, I have updated the CITATION.cff
N/a. If this PR edits table definitions, I have included an alter snippet for release notes.
N/a. If this PR makes changes to position, I ran the relevant tests locally.
If this PR makes user-facing changes, I have added/edited docs/notebooks to reflect the changes
I have updated the CHANGELOG.md with PR number and description.

codecov · 2025-08-20T20:22:39Z

Codecov Report

❌ Patch coverage is 85.99641% with 78 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.24%. Comparing base (8c6ad08) to head (87c15b2).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
src/spyglass/utils/mixins/ingestion.py	83.45%	23 Missing ⚠️
src/spyglass/common/common_ephys.py	57.69%	22 Missing ⚠️
src/spyglass/spikesorting/imported.py	62.50%	9 Missing ⚠️
src/spyglass/common/populate_all_common.py	81.25%	6 Missing ⚠️
src/spyglass/common/common_device.py	96.39%	4 Missing ⚠️
src/spyglass/common/common_dio.py	83.33%	4 Missing ⚠️
src/spyglass/common/common_interval.py	89.18%	4 Missing ⚠️
src/spyglass/common/common_subject.py	86.95%	3 Missing ⚠️
src/spyglass/common/common_lab.py	98.59%	1 Missing ⚠️
src/spyglass/common/common_nwbfile.py	66.66%	1 Missing ⚠️
... and 1 more

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1377      +/-   ##
==========================================
+ Coverage   71.02%   71.24%   +0.21%     
==========================================
  Files         113      114       +1     
  Lines       13088    13243     +155     
==========================================
+ Hits         9296     9435     +139     
- Misses       3792     3808      +16

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

src/spyglass/common/common_subject.py

CBroz1

Thanks for taking this on - great start!

src/spyglass/common/common_subject.py

CBroz1 · 2025-08-26T21:29:31Z

src/spyglass/utils/__init__.py

+    "SpyglassIngestion",
+    "SpyglassMixinPart",


What do we want the eventual list to be? I'm thinking through the naming convention here...

It seems like we're mixing type nouns with process verbs:

Type noun: SpyglassMixin, SpyglassMixinPart (eventually SpyglassMixinParams?)

Verb process: SpyglassIngestion

So change to SpyglassImported?¹ SpyglassIngested?

What do we want the common prefix to be?

SpyglassMixin (or SpyglassMixinTable), SpyglassMixinPart, SpyglassMixinIngestion

SpyglassMixin, SpyglassPart, SpyglassIngestion

I prefer the latter, where Mixin implies general

I would ideally like to ...

class SpyglassParams(SpyglassMixin, dj.Lookup): ... class SomeParams(SpyglassParams): ...

But this wouldn't work retroactively. If you change a table type, it just declares a new one. Related issue

Footnotes

I don't love this because it implies dj.Imported tables, which these are not ↩

To match #1435 we can make this IngestionMixin and move to it's own .py file

I think the pattern I went with in 1435 was to have (1) private mixins called XMixin: ExportMixin, FetchMixin, etc., and (2) public mixins called SpyglassX: SpyglassMixin, SpyglassAnalysis

Up to you how much you want to put in utils/mixins/ingestion.py versus utils/dj_mixin.py

src/spyglass/utils/dj_mixin.py

CBroz1 · 2025-08-26T21:35:53Z

src/spyglass/utils/dj_mixin.py

+        The reserved key "self" refers to the original object.
+        Additional keys can be added to access data from other nwb objects that are
+        attributes of the object (e.g. device.model).


I didn't see a case that had additional keys. Could you flush out the 'device.model' example a bit more?

Some tables need to access information in an attribute of an attribute which this gives a format to do.

Alternatively, Since I later added the option for functions as the value those cases could be handled that way instead

CBroz1 · 2025-08-26T21:39:10Z

src/spyglass/utils/dj_mixin.py

+    def _source_nwb_object_type(self):
+        """The type of NWB object to import from the NWB file.
+
+        If None, the table is either incompatible with NWB ingestion or must implement


This seems like an odd case to me: a table that inherits this class but is incompatible with nwb ingestion? Could that happen?

Oops, that was the docstring before I pulled it out into it's own class (rather than part of SpyglassMixin). Should be removed

src/spyglass/common/common_session.py

src/spyglass/common/common_subject.py

samuelbray32 · 2025-09-02T18:51:18Z

Benefits of standardization: Can auto-generate doc table of the ingestion mapping

* Auto-gen mapping doc * Refactor SpyglassIngestion * execute_inserts -> dry_run * Handle inserts as dict for parts. Quiet ingest logging * Minor edits * Add load config * Prevent prompt on equivalent entries. Always handle entries as dict * Fix ingestion order * Add DIOEvents * Remove debug print * Pluralize 'adjust_keys'. Add Raw * Update changelog * Fix tests * Remove comment * Apply suggestions from code review Co-authored-by: Copilot <[email protected]> * Apply suggestions from code review 2 * IntervalList.insert -> cautious_insert * Fix recursive call * Fix restrict by UUID --------- Co-authored-by: Copilot <[email protected]>

Copilot

Pull Request Overview

This PR standardizes NWB ingestion across Spyglass by implementing a centralized SpyglassIngestion class. The new approach replaces individual insert_from_nwbfile methods with a unified framework that maps NWB object attributes to table keys using declarative properties.

Key changes:

Introduces SpyglassIngestion mixin class with standardized ingestion workflow
Converts 14+ tables to use the new ingestion system (Institution, Lab, LabMember, Subject, Device tables, Session, Raw, DIOEvents, etc.)
Replaces legacy make methods with insert_from_nwbfile calls and deprecation warnings

Reviewed Changes

Copilot reviewed 24 out of 24 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
src/spyglass/utils/dj_mixin.py	Adds `SpyglassIngestion` class with core ingestion framework
src/spyglass/common/common_*.py	Converts multiple tables to use `SpyglassIngestion` pattern
src/spyglass/data_import/insert_sessions.py	Updates session insertion to support reinsert parameter
tests/	Updates tests to use new ingestion methods
docs/	Adds auto-generated ingestion mapping documentation

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

src/spyglass/common/common_subject.py

Copilot · 2025-10-14T18:27:42Z

src/spyglass/common/common_subject.py

+        # Avoids triggering 'accept_divergence' on reinsert
+        adjusted = []
+        for key in keys.copy():
+            key["sex"] = self.standardized_sex_string(key, warn=False)


The standardized_sex_string method expects a subject object as first parameter, but key is a dictionary. This will cause a runtime error.

src/spyglass/common/common_lab.py

src/spyglass/utils/dj_mixin.py

Copilot · 2025-10-14T18:27:43Z

src/spyglass/common/common_ephys.py

                warnings.warn(
                    f"Electrode ID {nwbfile_elect_id} exists in the NWB file "
                    + "but has no corresponding config YAML entry."
                )


[nitpick] The logic is inverted from the original. Consider adding a comment explaining why electrodes without config entries are skipped to improve code clarity.

src/spyglass/common/common_interval.py

…yglass into standard_import

…to standard_import

CBroz1 · 2025-10-31T15:05:44Z

src/spyglass/common/common_device.py


 @schema
-class DataAcquisitionDeviceSystem(SpyglassIngestion, dj.Manual):
+class DataAcquisitionDeviceSystem(SpyglassMixin, IngestionMixin, dj.Manual):


I think I prefer SpyglassIngestion as the interface for consistency....

class SomeTable(SpyglassMixin, dj.Something) class OtherTable(SpyglassAnalysis, dj.Something) Class ThirdTable(SpyglassIngestion, dj.Something)

Currently, the analysis code is organized as ...

utils/mixins/analysis.py::AnalysisMixin utils/dj_mixin.py::SpyglassAnalysis

It makes sense to me that this would be ...

utils/mixins/analysis.py::IngestionMixin utils/dj_mixin.py::SpyglassIngestion

Keep the custom class inheritance confined to dj.Mixin for simplicity? Allows us to manage the MRO behind the utils curtain, and prevent able classes from bloating out at the declaration

That's reasonable. I'll make the switch

CBroz1

Just one minor thing of adding the new docs to mkdocs.yml

CBroz1 · 2025-10-31T18:43:51Z

docs/mkdocs.yml

    - Understanding a Schema: ForDevelopers/Schema.md
    - Custom Pipelines: ForDevelopers/CustomPipelines.md
    - Using NWB: ForDevelopers/UsingNWB.md
+    - Ingestion Mapping: ForDevelopers/ingestion_mapping.md


Please also add features/Ingestion.md

initial version of standard import steps

635859a

samuelbray32 mentioned this pull request Aug 20, 2025

Data import conventions: Imported vs insert_from_nwb #1375

Open

edeno requested a review from CBroz1 August 21, 2025 11:58

CBroz1 mentioned this pull request Aug 21, 2025

Optogenetics metadata #1312

Merged

7 tasks

samuelbray32 added 10 commits August 22, 2025 08:42

add new standard for ingesting configs

71f885d

debug and make SpyglassIngestion class

5ec7b77

transition CameraDevice for new mixin

d034067

cleanup commented lines

928c05f

accomodate tables that insert into parts in mixin

db68310

transition common lab ingestion to new standard

641764b

adjust calls in Session population for new standard

ad9965e

remove commented lines

904f8a6

migrate table to new ingestion format

62ead34

move subject to new import

29f7384

github-advanced-security bot found potential problems Aug 26, 2025

View reviewed changes

src/spyglass/common/common_subject.py Fixed Show fixed Hide fixed

CBroz1 reviewed Aug 26, 2025

View reviewed changes

samuelbray32 added 10 commits August 29, 2025 11:29

add validation of secondary keys for duplicates

dbabfab

update Probe tables to new ingestion

97f367a

for table objects, have defaut get entries run on each row

40f848e

move interval list to new ingestion

5babaee

Move table inserts out of Session.make and run in populate_all_common

066c236

Migrate session to new ingestion scheme

0a9ffc9

update method call in test_lab_member_insert_file_str

d21c83f

restore imports for table definitions

6a16b6a

add lab institute and subject data to mock dio file

0a395bb

spelling

4c86bbe

initial version of auto doc markdown table

b8d45df

CBroz1 mentioned this pull request Sep 10, 2025

Ingest trials and structured behavior #1349

Draft

13 tasks

CBroz1 and others added 4 commits October 10, 2025 13:41

IntervalList.insert -> cautious_insert

a84cc58

Fix recursive call

225216a

Fix restrict by UUID

9166d4e

edeno marked this pull request as ready for review October 14, 2025 18:25

edeno requested review from CBroz1, Copilot and edeno October 14, 2025 18:25

Copilot AI reviewed Oct 14, 2025

View reviewed changes

CBroz1 mentioned this pull request Oct 15, 2025

The centrality of AnalysisNwbfile prevents table declaration during transactions #1434

Closed

CBroz1 mentioned this pull request Oct 24, 2025

Nwb-object oriented ingestion #1326

Open

31 tasks

CBroz1 and others added 7 commits October 24, 2025 15:45

Merge branch 'standard_import' of https://github.com/LorenFrankLab/sp…

b90a1fb

…yglass into standard_import

Revise based on code review

f863d16

Merge branch 'master' of https://github.com/LorenFrankLab/spyglass in…

d2b9667

…to standard_import

interval_name_from_tags fallback

34023c7

WIP: fix boolean condition of index==0

02d684a

WIP: fix interval typing

a477275

Merge branch 'master' into standard_import

6e6c3a5

samuelbray32 force-pushed the standard_import branch from 00a52b7 to 6e6c3a5 Compare October 30, 2025 16:40

samuelbray32 added 4 commits October 30, 2025 09:45

cleanup mixin merge

c0d5b77

rename to IngestionMixin and move to mixin folder

f5e051e

fix import location

565f454

update docs

20765be

samuelbray32 changed the title ~~WIP: Standardize nwb ingestion~~ Standardize nwb ingestion Oct 30, 2025

spelling fix

641bcd7

CBroz1 reviewed Oct 31, 2025

View reviewed changes

samuelbray32 added 2 commits October 31, 2025 08:26

Merge branch 'master' into standard_import

7d966f5

change to SpyglassIngestion = SpyglassMixin + IngestionMixin

ff01853

CBroz1 approved these changes Oct 31, 2025

View reviewed changes

update docs yaml

87c15b2

Standardize nwb ingestion #1377

Are you sure you want to change the base?

Standardize nwb ingestion #1377

Conversation

samuelbray32 commented Aug 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Known Tables for Ingestion

Checklist:

Uh oh!

codecov bot commented Aug 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

CBroz1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

samuelbray32 commented Sep 2, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Copilot AI Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Oct 14, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CBroz1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

samuelbray32 commented Aug 20, 2025 •

edited

Loading

codecov bot commented Aug 20, 2025 •

edited

Loading