Add ndtypes from ndx-pose, ndx-photometry, ndx-fiber-photometry

[rly]

In response to a Slack thread, this PR adds mappings for some neurodata types from ndx-pose, ndx-photometry (deprecated), and ndx-fiber-photometry. I wasn't sure whether there would be issues with the same neurodata type name from two different modules being mapped. I also made up the approaches and techniques...

It also fixes the casing of "OptogeneticStimulusSIte" -> OptogeneticStimulusSite"

[rly]

@satra pointed me to:
https://github.com/BICCN/TMN/blob/main/templates/approaches_template.csv for approaches

I did not find a generic controlled vocabulary for techniques. Are you using https://github.com/BICCN/TMN/blob/main/templates/SPARC%20Modalities.csv ?

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 77.33%. Comparing base (d91a041) to head (bfbe6eb).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1665   +/-   ##
=======================================
  Coverage   77.33%   77.33%           
=======================================
  Files          89       89           
  Lines       13325    13325           
=======================================
  Hits        10305    10305           
  Misses       3020     3020           

Flag	Coverage Δ
unittests	77.33% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 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.

[satra]

@rly - this is a lot less formal at this point. if you see the module list you will see how we were approaching the classification for that. the main thing this is being used for is the summary info in the dandiset and for the CLI to generate meaningful suffices when disambiguating. if you have suggestions for changes to the others, please make them.

for example ndx-spectrum seems off, and incorrect. the modality field should be on alphabetical only without any dashes.

i believe we did this by taking all the dandisets we had 4+ years ago and extending this list.

[satra]

on a more general note, i'm a bit confused about why pose training is a data type, as that doesn't seem to reflect behavior per se, and also why it is a pose estimation technique.

some of these are direct measurement techniques or estimation techniques, but this one seems odd.

[rly]

PoseTraining is a group consisting of training frames and source videos. A training frame refers to a frame from a source video and an the locations of each node of 1 or more skeletons, often from human labeling. So PoseTraining isn't exactly a data type but indicative of the NWB file containing training data for pose estimation methods. Should I pick out a different data type to detect instead?

[satra]

we should consider data type as a broad umbrella of what data is generated from or used for (for example there is fourier analysis above). it could be associated with a "pose estimation training technique"

[rly]

got it. that makes sense

[yarikoptic]

@rly here we have 2 extensions listed to assist with importing https://github.com/dandi/dandi-cli/blob/HEAD/dandi/metadata/nwb.py#L99 -- should we also add extension information into this 'registry' and use it there too? or may be it is no longer even needed?

[yarikoptic]

so -- what was the verdict on this one -- remove this entirely or just adjust technique or ... ?

[rly]

@rly here we have 2 extensions listed to assist with importing HEAD/dandi/metadata/nwb.py#L99 -- should we also add extension information into this 'registry' and use it there too? or may be it is no longer even needed?

That very limited registry is no longer needed, because not long after NWB 2 was released, the NWB APIs started caching the specs used, including extensions, by default in the file, which makes those files readable without external packages. I sampled from the 20 lowest-numbered dandisets and found only two that do not have cached schema: 000004 and 000019. Neither uses extensions. Other extensions in those dandisets (ndx-dandi-icephys, ndx-aibs-ecephys, ndx-mies, ndx-grayscalevolume, ndx-icephys-meta, ndx-spectrum, ndx-events) have the extension cached, so an extra import is not necessary.

I think we should remove the registry and loop there.

Also, ndx-hed and ndx-events can be removed from pyproject.toml. They should not be needed unless dandi-cli is using convenience functions from them.

[rly]

I adjusted the PoseTraining mapping "technique" to "pose estimation training technique". That's good to me.

[yarikoptic]

agreeing with @satra on avoiding - and also overall we should aim to make them shorter

[rly]

It's probably worthwhile also to reindex all the neurodata types observed on DANDI and add those to the map, but this PR is a start and addresses the issue raised over Slack.

[yarikoptic]

there are errors from docker compose but also smth which is relevant to this PR to be addressed

FAILED dandi/tests/test_metadata.py::test_ndtypes[ndtypes34-asset_dict34] - AssertionError: assert 'calcium imag...ation imaging' == 'fiber photom...cal technique'
  
  - fiber photometry technique; optical technique
  + calcium imaging; cell population imaging

re @rly's

It's probably worthwhile also to reindex all the neurodata types observed on DANDI and add those to the map, but this PR is a start and addresses the issue raised over Slack.

do we have enough extracted in @magland's lindi's to just grep through them?

[rly]

From https://neurosift.app/dandi > "Neurodata Types Search", you can see a dropdown menu of all of the parsed neurodata types.

That is populated from parsing https://lindi.neurosift.org/dandi/neurodata_types_index.json.gz

Generated by https://github.com/magland/neurosift-kerchunker/blob/main/workflow_scripts/create_neurodata_types_index.py

I believe these come from the first 100 assets of each dandiset, only from dandisets where at least one of the first 20 assets is NWB, and only if LINDI files have been generated for the NWB files (and other edge case handling).

[rly]

I'll try having an AI generate the

name {
        "module": x,
        "neurodata_type": x,
        "technique": x,
        "approach" x
}

mappings tomorrow and see how it goes.

[rly]

I added a new neurodata_type_map.py file with a more complete mapping of neurodata types (as detected by neurosift, as described above). Before I integrate it, update tests, etc., please take a look and let me know what you think. It should probably be its own file because it's so big. Should it be a python dictionary? JSON? Also, what is the difference between the key and the value of "neurodata_type"?

[yarikoptic]

dandi/metadata/neurodata_typemap.py seems ot be note used, then what is it for?

type checking seems to fail.

I will move it to draft for now, but I would love for us to finalize and merge this notable improvement!!

[yarikoptic]

@rly with an interest in physio recordings and thus potentially in https://github.com/BCM-Neurosurgery/ndx-wearables I would like to bring this over the finish line. Worse comes to worse we should do that while on @bendichter's couch in the booth at SfN!

[bendichter]

I would like to help bring this over the finish line, and I think the way to do that is to land the asset metadata mapping first and split the filename suffix work into a follow-up. Some observations from looking at the current state of the PR and at what is deployed on the archive.

On why the metadata half matters on its own: I scanned the assetsSummary of all 895 dandisets, and the only approach and measurementTechnique values in the entire archive are the ones this map emits. The photometry and pose dandisets carry no marker at all, so there is no facet or query that can find them (see #1754, which this PR would address).

On scope: nothing in the codebase currently reads the module field of this map. process_ndtypes uses only approach and technique, and the _ecephys-style filename suffix in organize.py comes from _populate_modalities, which derives modalities by introspecting pynwb submodules and is untouched here. So the misc to sortedunits rename and the _units suffix discussion above will have no effect until organize.py is changed to consult this map. That is a small, separable change, and I suggest we do it in a follow-up PR rather than hold this one on it.

On the semicolon-joined terms: "fiber photometry technique; optical technique" becomes a single MeasurementTechniqueType name, not two techniques, so nothing faceting on "optical technique" would ever match it. The existing "microscopy approach; cell population imaging" set that precedent, but I do not think we should extend it. The schema already models both approach and measurementTechnique as lists at the asset and summary level, so no dandischema change is needed: we can make the typemap values lists (technique: list[str]) and have process_ndtypes add each term individually.

On the photometry approach term: "calcium imaging; cell population imaging" is wrong for a large fraction of photometry dandisets, since dLight, GRAB-DA, and GRAB-ACh sensors measure neurotransmitters rather than calcium (000251, 000351, and 001434 are examples). I would use "fiber photometry approach", which is accurate regardless of indicator and follows the pattern of "optogenetic approach".

On the new dandi/metadata/neurodata_typemap.py: as @yarikoptic noted, it is not imported anywhere, and it duplicates the map in util.py, so the two would immediately drift. I would either make it the single source of truth that util.py imports from, or drop it from this PR and handle the expanded map separately. If we keep it, note that matching is by bare type name, so two extensions defining the same type name will collide silently in the dict literal.

The one failing test looks like the map and the test expectation simply have approach and technique swapped for FiberPhotometryResponseSeries.

@rly happy to push these changes to this branch if that would help.

[bendichter]

I implemented the above in a PR against this branch: rly#1. It switches the photometry entries to "fiber photometry approach" / "fiber photometry technique" as plain single terms (with single terms per type, no change to the map structure or to dandischema is needed), adds the table and device types from both photometry extensions, and drops the unused neurodata_typemap.py for now. @rly if you merge it into add_ndtypes it will show up here.

[rly]

Yes, let's get this over the finish line! 🏁

Thanks @bendichter for suggesting we drop the much bigger refactor of the mapping of neurodata types, which requires larger discussion and agreement. Let's focus on getting these extension data types in and correcting longstanding issues.

I reverted the changes to the module mapped to Units and Spectrum since apparently module is unused and suffix is determined by get_neurodata_types_to_modalities map, which uses the pynwb module hierarchy of the associated pynwb class. (Aside: this code is incomplete and misses all DynamicTable subtypes. I'll create a separate issue for this.)

We'll fix up Units module "misc" -> "sortedunits" and "Spectrum" module "ndx-spectrum" -> "spectrum" in a separate PR where module might end up actually being used.

Updated PR summary

1. Add "technique": "optogenetic stimulation technique" to the OptogeneticSeries mapping, a term not previously used in the map.

2. Fix typo "OptogeneticStimulusSIte" -> "OptogeneticStimulusSite" and add "technique": "surgical technique". The typo never matched real data, so newly extracted files containing an OptogeneticStimulusSite gain approach: "optogenetic approach", measurementTechnique: "surgical technique", and variableMeasured: "OptogeneticStimulusSite". The first two may already be present from other types (OptogeneticSeries, ElectrodeGroup); the variableMeasured entry is always new.

3. Add types from ndx-pose, the deprecated ndx-photometry, and ndx-fiber-photometry. All photometry types get "approach": "fiber photometry approach", a new term. FibersTable and FiberPhotometryTable get "technique": "surgical technique" since they carry stereotactic implant coordinates, matching ElectrodeGroup. The response series and the commanded voltage series get a new "technique": "fiber photometry technique".