Make a SpeciesRecord dataclass for species_map records

[adityasingh2400]

Fixes #1867

species_map was a list of loose 4-tuples, which is what @CodyCBakerPhD flagged in the #1866 review. That prerequisite landed on 2026-06-01, so this is now unblocked.

Each entry becomes a frozen SpeciesRecord dataclass with common_names, prefix, uri, and name. __post_init__ enforces the invariants that until now only test_species_map checked: common names and prefix lower-cased, URI an NCBITaxon PURL, and name formatted as {scientific name} - {GenBank common name}. A malformed entry now fails at import rather than only under pytest.

The matching logic moves onto the record as matches_name and matches_common_name, so extract_species reads as the two-pass lookup it already was. The name.partition(" - ") calls are replaced by scientific_name and genbank_common_name properties. The separator element that partition returned could never equal a stripped input, so behavior is unchanged.

Tested with 3 new tests (6 cases with parametrization), all carrying @pytest.mark.ai_generated per CLAUDE.md, plus test_species_map reworked to take records. Full dandi/tests/test_metadata.py run gives 142 passed, 1 xfailed, 8 xpassed.

Since this is a refactor, an import failure alone would be weak evidence, so the behavior change was checked directly against the base ref. On master a malformed entry is accepted silently. On this branch it is rejected with Common name 'Mouse' of http://example.com/not-ncbitaxon must be lower-cased.

AI assistance disclosure: this change was written with the help of Claude Code, and the added tests are marked ai_generated as CLAUDE.md asks. I reviewed and tested everything before submitting.

[codecov]

Codecov Report

❌ Patch coverage is 77.04918% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.91%. Comparing base (2b5f8ea) to head (a1c4230).

Files with missing lines	Patch %	Lines
dandi/metadata/util.py	61.11%	14 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1899      +/-   ##
==========================================
- Coverage   76.96%   76.91%   -0.05%     
==========================================
  Files          88       88              
  Lines       12882    12927      +45     
==========================================
+ Hits         9914     9943      +29     
- Misses       2968     2984      +16     

Flag	Coverage Δ
unittests	76.91% <77.04%> (-0.05%)	⬇️

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.

[yarikoptic]

THANK YOU for the PR @adityasingh2400 ! Overall looks good and worth pursuing. With claude we seems have identified a number of concerns which I will post now. See (with your claude ;) ) the about to be posted review.

[yarikoptic-gitmate]

Nice refactor — this is the right shape for #1867, and moving the matching onto the record makes extract_species read as the two-pass lookup it always was. I checked it out and verified the behavior-preservation claim rather than taking it on faith:

• pytest dandi/tests/test_metadata.py → 143 passed, 1 skipped, 8 xpassed
• flake8 clean; black --check unchanged
• mypy: 5 errors, all missing-dateutil-stub errors in dandi/utils.py, dandi/tests/fixtures.py, and dandi/tests/test_metadata.py:31 — identical set on master, so nothing introduced
• Matching equivalence: I reimplemented the old partition-based predicate and diffed it against matches_name/matches_common_name over every name, name-half, common name and prefix in the map, plus edge inputs ("", " ", "-", " - ", case and whitespace variants) → 0 mismatches

Your reasoning about partition's separator element is right, and for a second reason worth stating: partition(" - ")[1] is always " - ", and lower_value is .strip()ed, so it can never match regardless of the data.

Requesting changes on one class of issue: __post_init__ is now the sole guardian of these invariants, but it accepts several malformed entries that are worse than the ones it rejects. Since the stated goal is "a malformed entry now fails at import rather than only under pytest," it's worth closing those before this lands.

Must fix

1. prefix="" and common_names=("",) are accepted, and either one breaks the whole table

SpeciesRecord(("mouse",), "", uri, "Mus musculus - House mouse")   # ACCEPTED
SpeciesRecord(("",), "mus", uri, "Mus musculus - House mouse")     # ACCEPTED

"" == "".lower() passes the lower-case check, and value.startswith("") is True for every input — so that record matches everything, and every other lookup starts failing with "Got multiple (N) species matched … Should not happen." Verified: an empty-prefix record returns True for matches_name("zebrafish").

This is a bigger hole than anything __post_init__ currently catches. Please reject empty prefix and empty entries in common_names.

2. common_names type isn't enforced, and the likely mistake is silent

Five of the thirteen entries are single-element tuples, so the realistic error is a dropped trailing comma:

SpeciesRecord(("mouse"), "mus", ...)   # common_names == "mouse", a str

That constructs, passes __post_init__ (iterating a lowercase string yields lowercase chars), and is hashable — no loud failure anywhere. I patched it in and ran it:

'm'      -> Mus musculus - House mouse
'e'      -> Mus musculus - House mouse
'mouse'  -> ERROR: Cannot interpret species field: mouse

Single letters resolve to mouse; the real common name stops resolving entirely. And test_species_map passes anyway, because chain(...) iterates the string's characters and every one of them now "matches."

The one thing that caught it was assert isinstance(record.common_names, tuple) in your new test_species_map_entries_are_records — so that assertion is genuinely load-bearing, please keep it. But it should be a guarantee rather than a spot-check: either validate the type in __post_init__, or normalize with object.__setattr__(self, "common_names", tuple(self.common_names)). Worth a case in test_species_record_rejects_malformed_entry too, which currently has no bad-common_names-type case.

3. The URI check is prefix-only

if not self.uri.startswith(NCBITAXON_URI_TEMPLATE.format("")):

All of these are accepted: NCBITaxon_, NCBITaxon_abc, NCBITaxon_9606/junk, NCBITaxon_9606extra. That matters because extract_species matches incoming URIs with NCBITaxon_([0-9]+), so a non-numeric entry constructs fine and is then silently unreachable via the URI path.

Suggest validating the taxon id is numeric — but please don't do it with a second hardcoded copy of the URL, which would defeat the point of NCBITAXON_URI_TEMPLATE. Cleanest is probably to store the numeric id on the record and derive uri from the template.

4. The comment on the hash assertion is inaccurate

# frozen dataclasses are hashable, which `extract_species` relies on
# indirectly when de-duplicating matches
assert hash(record) == hash(record)

extract_species de-duplicates list(set(value_matches)) where the elements are (uri, name) string tuples — records are never hashed, so the stated dependency doesn't exist.

On the assertion itself: it isn't reading a cached value (the generated __hash__ rebuilds and rehashes the field tuple on every call), but with all fields being str/tuple[str, ...]/None it can only ever fail by raising, which a bare hash(record) detects identically. And the hash isn't reproducible across runs — PYTHONHASHSEED randomizes str hashing, and nothing here pins it or needs it to be stable.

The eq/hash contract is worth locking in, so rather than dropping the line, suggest strengthening it to two distinct-but-equal instances:

copy = dataclasses.replace(record)
assert copy is not record
assert copy == record and hash(copy) == hash(record)

That version additionally catches eq=False or a hand-rolled identity __hash__, which the current form passes silently.

Should fix

5. matches_name / matches_common_name are public with an unenforced case contract

species_map[0].matches_name("Mus musculus")     # False
species_map[0].matches_common_name("Mouse")     # False

The refactor promoted an inline predicate that always received pre-normalized input into public API that doesn't normalize, with only docstring prose to warn callers. Either normalize inside the methods, or make them _matches_name/_matches_common_name.

6. No direct tests of the two methods the refactor exists to create

The new tests cover the name-half properties and the four rejection paths, but matches_name's prefix branch is exercised only transitively through extract_species. A direct test would also usefully document the surprising bit — matches_name("mushroom") is True for Mus musculus.

7. __post_init__ accepts degenerate names

" - Human" and "Human - " both pass. The first yields scientific_name == "", which makes matches_name("") return True — and extract_species guards value_orig != "" but not whitespace-only, so {"species": " "} reaches it. Not live with the current data, but the check is "separator present," not "both halves non-empty."

8. Docstring nit

matches_name's "or its prefix" reads as equality; it's startswith. The class docstring gets this right — worth matching. Also worth a one-line note on genbank_common_name that it relies on __post_init__ having validated the separator, since it would IndexError otherwise.

Optional

• Cross-record invariants are the ones that actually reach users. Nothing checks for duplicate uris or a prefix that matches another record. Adding prefix="ma" to Macaca mulatta, for example, breaks radiata and nemestrina (mulatta itself keeps resolving, since the conditions OR within one record). No collisions exist today — this is hardening, not a live bug — but a module-level check or an extra test would cover a class of mistake that per-record lower-casing doesn't.
• species_map is still a mutable list of frozen records; species_map: tuple[SpeciesRecord, ...] would match the intent.
• In test_species_map, assert key.lower() == key and the separator assertion can no longer fail — __post_init__ guarantees them at import. Worse, the first also passes for the bare-string case in #2, so it's misleading about what it protects.
• matches_common_name could just be value in self.common_names.

None of this changes the verdict — the refactor is behavior-preserving and the validation is a real improvement over what only test_species_map checked before. Mostly it's that __post_init__ should reject the entries that would do the most damage.

---

Generated by Claude Code

[adityasingh2400]

Thank you for this, particularly for reimplementing the old predicate and diffing it rather than taking the equivalence claim on faith. All six points are addressed in 9b06d87.

1. Empty prefix and empty common_names entries. Both rejected now. You are right that this was the biggest hole, since value.startswith("") is true for everything and one such record would turn every other lookup into "Got multiple (N) species matched".

2. common_names type. Rejected rather than coerced, and your framing of the realistic error was the useful part. I want to flag one thing about the suggested alternative: object.__setattr__(self, "common_names", tuple(self.common_names)) would not have caught it, because tuple("mouse") is ('m','o','u','s','e'), which is exactly the character tuple that causes the bug. So it had to be a type check. There is now a dedicated test for it, and the isinstance assertion you called load-bearing stays.

3. URI check. Taken your suggestion rather than patching the prefix check. The record now stores taxon_id and derives uri from NCBITAXON_URI_TEMPLATE, so there is no second copy of the URL and a malformed PURL is unconstructible instead of merely rejected. taxon_id is validated as numeric, which is the property extract_species actually depends on through NCBITaxon_([0-9]+). The 13 entries carry the bare id now.

4. Hash assertion. You are right that the comment described a dependency that does not exist, extract_species de-duplicates (uri, name) string tuples. Replaced with the two-instance form you suggested, and the comment now says what it actually locks in.

5. Case contract. Both methods normalize their input now, so the contract is enforced rather than described. I chose that over renaming to _matches_* because the methods are useful to callers and normalizing removes the trap entirely.

6. Direct tests. Added, including matches_name("mushroom") being true for Mus musculus, since you are right that the surprising part deserves to be written down rather than left implicit.

On verification, one caveat worth stating plainly. dandischema and hdmf are not installed in this environment, so I could not run pytest dandi/tests/test_metadata.py the way you did. What I did instead was execute the SpeciesRecord class standalone against the constants and check every case directly. All five new rejections raise with the expected message, uri derives correctly, the two-instance eq/hash holds, and the normalizing and negative cases behave. black and flake8 are clean, including an unused NCBITAXON_URI_TEMPLATE import in the tests that fell out of the taxon_id change. I am relying on CI for the full suite.

[yarikoptic-gitmate]

All six are genuinely addressed in 9b06d87 — I re-verified each against the new code rather than reading the diff. And you're right about object.__setattr__(..., tuple(self.common_names)): tuple("mouse") is ('m','o','u','s','e'), which is precisely the character tuple that causes the bug, so coercion would have laundered the mistake instead of catching it. The type check was the only correct fix; my suggestion was wrong on that branch.

Since you couldn't run the suite locally, here are the results from an environment that has dandischema/hdmf:

pytest dandi/tests/test_metadata.py   148 passed, 1 skipped, 8 xpassed
black --check                          unchanged
flake8                                 clean
mypy                                   5 errors, all pre-existing missing-dateutil-stub
                                       errors on untouched lines; identical on master

Re-verified directly:

• Equivalence still holds. The methods normalize now, so I re-ran the old partition-based predicate against the new matches_name across every name, half, common name, prefix and the whitespace/case edge cases — 0 mismatches. extract_species already passed value_orig.lower().strip(), so the added .strip().lower() is idempotent there and nothing shifts.
• All five holes closed, with the messages the tests expect: empty prefix, empty common name, taxon_id="abc", taxon_id="", and common_names="mouse" (the last as TypeError: ... must be a tuple, got str).
• uri derivation produces a well-formed PURL for all 13 entries. Moving it to a property also means eq/hash now key on taxon_id, which is equivalent and keeps dataclasses.replace working — the new two-instance assertion passes.

One thing that will fail CI

import dataclasses landed in the third-party block rather than the stdlib group, and isort runs in the lint pre-commit job:

--- dandi/tests/test_metadata.py:before
+++ dandi/tests/test_metadata.py:after
@@ -1,5 +1,6 @@
 from __future__ import annotations

+import dataclasses
 from datetime import datetime, timedelta
 from itertools import chain
@@ -33,8 +34,6 @@
 from pynwb import NWBHDF5IO, NWBFile, TimeSeries
-import dataclasses
-
 import pytest

black and flake8 don't catch import ordering, which is why it looked clean — isort --check-only dandi/tests/test_metadata.py, or just pre-commit run --all-files, reproduces it.

Optional nit

taxon_id.isdigit() is a hair looser than the [0-9]+ it's standing in for — "²".isdigit() and "٢٥".isdigit() are both True, and neither would match NCBITaxon_([0-9]+) on the way back in. isdecimal() doesn't close it either ("٢٥".isdecimal() is True); taxon_id.isascii() and taxon_id.isdigit(), or re.fullmatch(r"[0-9]+", taxon_id), is exact. Purely theoretical for a hand-maintained table — mentioning it only because the stated rationale was matching what extract_species actually depends on.

Everything I raised is resolved. Fix the import order and this looks good to me; I'll leave the formal approval to @yarikoptic.

---

Generated by Claude Code

[adityasingh2400]

Fixed in 321479c, and thank you for running the suite and posting the numbers. That fills exactly the gap I flagged.

import dataclasses now sits in the stdlib group between from __future__ import annotations and from datetime import ..., matching the diff you posted. I checked it against the config rather than guessing: [tool.isort] uses profile = "black" with force_sort_within_sections = true, which sorts import x and from x import together by module name, so dataclasses belongs immediately before datetime. black and flake8 are still clean. isort itself is not installed here, so that placement is reasoned from the config and your diff rather than run locally.

On the coercion point, thank you for saying so directly. It would have been easy to apply the suggestion as written, and the result would have looked correct while quietly turning the exact bug we were fixing into the accepted state.

Two things from your notes I want to acknowledge rather than let pass silently:

The .strip().lower() being idempotent inside extract_species is the reason the normalization was safe to add, and you verified that rather than assuming it. I had reasoned it but not proven it against the old predicate.

Your point that moving uri to a property changes what eq and hash key on is a consequence I did not call out. Keying on taxon_id instead of the derived URI is equivalent because the mapping is total and injective, but it is a real change in what the dataclass compares, and it deserved a sentence in my previous comment.

Codecov flags 14 uncovered lines on the patch. Most of those are the new rejection branches, which the parametrized cases do exercise, so I suspect the report predates 9b06d87. Happy to look properly once CI has run against this head if it still shows the same.

[yarikoptic-gitmate]

Ran isort against 321479c so the placement isn't reasoned-only on either side — both touched files pass:

$ isort --check-only dandi/tests/test_metadata.py dandi/metadata/util.py
exit=0

Your reading of the config was right: force_sort_within_sections sorts import x and from x import ... together by module name, so dataclasses immediately before datetime is correct. Full suite on this head is still 148 passed, 1 skipped, 8 xpassed.

On codecov — your suspicion is right, it's stale. The comment names its head explicitly: "Comparing base (2b5f8ea) to head (a1c4230)", last updated 14:47Z, which is before both 9b06d87 (22:59Z) and 321479c. So those 14 lines are from the original commit and predate every rejection branch and direct test you added. It should refresh once CI runs against this head.

One caveat so you don't take a wrong number from me: I can't give you a trustworthy local patch-coverage figure to compare against. dandi/metadata/util.py is imported during collection, before --cov starts instrumenting, so a local run reports the class body and species_map as unhit when they obviously execute. Worth knowing if you go checking it yourself after CI — the honest read is to wait for codecov on the new head.

No further comments from me; the substance was settled in 9b06d87.

---

Generated by Claude Code

[adityasingh2400]

The Codecov comment above is stale and I do not think it should hold this up. It reports a1c4230, which is three commits behind the current head 321479c, and predates the validation tests I added in 9b06d87 for your review points.

Measured against the current head, all 58 lines this PR adds to dandi/metadata/util.py are covered, and the file is at 92.89%:

$ python -m coverage run --source=dandi.metadata.util -m pytest dandi/tests/test_metadata.py
dandi/metadata/util.py   380   27   92.89%

None of the 27 remaining misses fall inside this PR's diff. They are all pre-existing lines outside it.

Worth flagging one measurement trap, since it is what made me check twice. Running this through pytest --cov instead reports 75% and lists the whole species_map literal as uncovered. That is an artifact of coverage attaching after the module is already imported, visible in the report because it also marks the import block at lines 1 to 21 as missing. Starting coverage before the import gives the number above.

Separately, test_bids_nwb_metadata_integration fails locally for me with FileNotFoundError: 'bids-validator-deno'. That reproduces unchanged on the merge base, so it is my missing binary and not something from this branch.

[yarikoptic-gitmate]

Reproduced, independently, on 321479c — same command, same number:

dandi/metadata/util.py   380   27   92.89%

I also checked the stronger claim mechanically rather than by eye, intersecting the 27 uncovered lines against the set of lines the diff actually adds to dandi/metadata/util.py:

added lines in util.py: 158
uncovered lines total:  27
uncovered ∩ added:      NONE

So every line this PR adds is covered, and the remaining misses are all pre-existing. Your read of the measurement artifact matches mine — the giveaway really is lines 1–21 showing as missing, which can only mean the module was already imported when coverage attached.

On test_bids_nwb_metadata_integration: it passes here (63s), so your bids-validator-deno diagnosis is right — that's a missing local binary, not anything from this branch.

That closes the last open question from my side. Nothing outstanding.

---

Generated by Claude Code