Fix two by_name() edge cases and add case_sensitive and match="contained" options#100
Open
gefleury wants to merge 5 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.
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
1 participant
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