Fix two by_name() edge cases and add case_sensitive and match="contained" options - #100
Open
gefleury wants to merge 6 commits into
Open
Conversation
by_name(..., all=True) could return the same instance more than once, for two reasons: - an instance's own name-like properties (including synonyms) sharing a value (e.g. MolecularEntity.propofol) - match="contains" reaching the same instance via several distinct keys (e.g. searching SovereignState for "FR" matches France via both its "FR" and "FRA" synonyms) Deduplication preserves the original order, so the first match (all=False) is unaffected.
- hasattr(instance, prop_name) only checks whether the class defines the property, not whether this instance actually set it (every instance shares the same __init__ signature, with unset properties defaulting to None). - As a result, an unset property (e.g. ParcellationEntity without an abbreviation) was still indexed, leaving None as a lookup key. - match="contains" then crashed with TypeError, since `name in key` is invalid when key is None. - The fix only appends a name-like property to the lookup keys when its value is not None.
- by_name() gains a case_sensitive parameter (default True, preserving current behavior). - When False, equals/contains comparisons use casefold() instead of lower(), so Unicode variants like the micro sign "µ" and the Greek letter "μ" are also treated as equivalent. - Case-insensitive matching can merge results from genuinely different instances that only differ by case (e.g. MolecularEntity "pentobarbital" vs "pentobarbital sodium"). - Add tests reusing test_issue0069's License examples with flipped case, plus the MolecularEntity merge case and the Unicode casefold case.
by_name() gains match="contained", the reverse of "contains": finds instances whose name-like properties are substrings of the given string. Useful for NWB files: their fields often combine a canonical name with extra free text (e.g. "Mus musculus - House mouse"), embedding the openMINDS instance name inside a longer, non-canonical string.
Member
|
Nice work on the two bugfixes. |
apdavison
requested changes
Aug 7, 2026
apdavison
left a comment
apdavison
left a comment
Member
There was a problem hiding this comment.
Very nice PR. I have one proposed change: replace "contained" with "within".
As a follow-up, it might be nice to generalise case-sensitivity, and add an option to ignore accents - so that "Genevieve" would match "Geneviève"
| name (str): a string to search for. | ||
| match (str, optional): either "equals" (exact match - default) or "contains". | ||
| match (str, optional): either "equals" (exact match - default), "contains" | ||
| (the name-like property contains the given string), or "contained" |
Member
There was a problem hiding this comment.
I think "within" would be clearer than "contained" (which evokes the past-tense).
Author
|
Thank you both for the review and suggestions.
Let me know if I can merge this PR or whether you'd prefer me to include the accent handling here as well. |
apdavison
approved these changes
Aug 17, 2026
Member
|
@gefleury : looks good to me, I'm happy to leave accent handling for a separate PR. As a general rule, PRs should not be merged by their author. I'll leave it to @Raphael-Gazzotti to re-review and merge when ready. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Four small, independent changes to
by_name(), each with its own tests.Fixes
by_name(..., all=True)could return the same instance twice: when an instance's name-likeproperties share a value or when
match="contains"reaches it via several keys (bdec073).match="contains"raisedTypeErroron instances with unset name-like properties, which wereindexed under a
Nonekey (egParcellationEntityleavesabbreviationunset) (af66572).New options
case_sensitive(defaultTrue, existing behaviour unchanged) (70ce3e8).casefold(), so Unicode variants like "µ"/"μ" also match.case_sensitive=False,match="equals"can no longer use the dict lookup andscans all keys instead, so it drops from O(1) to O(n). That seems acceptable.
match="contained", the reverse of"contains": finds instances whose name-like properties aresubstrings of the given string. Motivated by NWB fields like "Mus musculus - House mouse" (056e80e).
Tests
Four tests added to
pipeline/tests/test_regressions.py