Skip to content

fix: keep every element when constructing controlled terms from a struct array - #109

Open
ehennestad wants to merge 1 commit into
add-graph-traversal-visitorfrom
fix-controlled-term-array-construction
Open

fix: keep every element when constructing controlled terms from a struct array#109
ehennestad wants to merge 1 commit into
add-graph-traversal-visitorfrom
fix-controlled-term-array-construction

Conversation

@ehennestad

@ehennestad ehennestad commented Aug 28, 2026

Copy link
Copy Markdown
Collaborator

A multi-valued property linking to controlled instances kept only its first entry when read back. An AtlasAnnotation with laterality set to both left and right reloaded with left alone. This is the defect the previous PRs listed as a known gap against 30 types.

Cause

Deserializing such a property assigns a struct array of references to it:

annotation.laterality = struct('at_id', {".../laterality/left", ".../laterality/right"});

MATLAB converts that by calling the controlled term constructor with the whole array. initializeControlledTerm handles it correctly at first glance:

numInstances = numel(instanceSpec);
if numInstances > 1
    obj(numInstances) = feval(class(obj));   % grow to one element per struct
end

But initializeControlledTerm is a method on a handle class that returned nothing. Growing obj inside it expands only the local copy; the caller keeps the original scalar handle it passed in, which is element one. Everything after the first element was built, populated and then discarded when the method returned.

The for loop below the expansion did populate each element, which is why this looked correct on inspection — the values existed, they just never left the method.

Fix

initializeControlledTerm returns the object array, and the three constructors that call it take the returned value. That covers the current ControlledTerm and the v2 and v3 variants under +abstract/private/controlledTerms.

Effect

The round-trip suite goes from 261 to 288 of 292 types, and 30 entries come off ommtest.helper.knownRoundTripGap.

What is left

The four remaining failures have nothing to do with each other and are now listed individually rather than as a group:

  • ParcellationTerminologyVersion and QuantitativeRelationAssessment fail during serialization, because a property holding an unresolved MixedTypeReference reaches openminds.internal.meta.fromInstance, and the meta type registry does not recognise it as a metadata type.
  • ChemicalSubstance fails because one of its controlled instances is named (N-methyl-(11c))2-(4''-methylaminophenyl)-6-hydroxybenzothiazole. Looking that up from a bare reference runs the name through matlab.lang.makeValidName, which mangles it beyond matching, so it reloads as an empty term with a fresh blank node identifier.
  • TermSuggestion fails because of the missing LINKED_PROPERTIES entry described in the controlled term deserialization PR.

Tests

Two cases in ControlledTermTest: one constructs the terms directly from a struct array, one goes through a property, which is the path deserialization actually takes. Both assert the element count and the identifiers, so a regression that silently drops entries is caught rather than a regression that merely changes a count.

🤖 Generated with Claude Code

@ehennestad
ehennestad force-pushed the fix-controlled-term-array-construction branch from f04c567 to a5ac5dd Compare August 28, 2026 00:15
@github-actions

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Test Results (R2022a)

750 tests  +2   749 ✅ +4   2m 34s ⏱️ -3s
 19 suites ±0     1 💤  - 2 
  1 files   ±0     0 ❌ ±0 

Results for commit 818b5de. ± Comparison against base commit 4fdcb07.

♻️ This comment has been updated with latest results.

@ehennestad
ehennestad force-pushed the fix-controlled-term-array-construction branch from 1fa140d to a5ac5dd Compare August 28, 2026 00:31
@codecov

codecov Bot commented Aug 28, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 33.33333% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.23%. Comparing base (4fdcb07) to head (818b5de).

Files with missing lines Patch % Lines
...stract/private/controlledTerms/v2/ControlledTerm.m 0.00% 1 Missing ⚠️
...stract/private/controlledTerms/v3/ControlledTerm.m 0.00% 1 Missing ⚠️
Additional details and impacted files
@@                       Coverage Diff                       @@
##           add-graph-traversal-visitor     #109      +/-   ##
===============================================================
+ Coverage                        79.20%   79.23%   +0.02%     
===============================================================
  Files                              419      419              
  Lines                             4083     4083              
===============================================================
+ Hits                              3234     3235       +1     
+ Misses                             849      848       -1     

☔ 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.

@ehennestad
ehennestad force-pushed the fix-controlled-term-array-construction branch 3 times, most recently from 5bca673 to a5ac5dd Compare August 28, 2026 12:59
@ehennestad
ehennestad force-pushed the fix-controlled-term-array-construction branch from fa864c8 to ff146ef Compare August 31, 2026 09:01
@ehennestad
ehennestad force-pushed the fix-controlled-term-array-construction branch from ff146ef to 495d4ec Compare August 31, 2026 10:39
@github-actions

github-actions Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Test Results (R2026a)

750 tests  +2   749 ✅ +4   2m 49s ⏱️ +5s
 19 suites ±0     1 💤  - 2 
  1 files   ±0     0 ❌ ±0 

Results for commit 818b5de. ± Comparison against base commit 4fdcb07.

♻️ This comment has been updated with latest results.

@ehennestad
ehennestad force-pushed the fix-controlled-term-array-construction branch from 495d4ec to 8a6c8c9 Compare September 1, 2026 14:31
…uct array

A multi-valued property that links to controlled instances kept only its
first entry when read back. An AtlasAnnotation with laterality set to
both left and right reloaded with left alone.

Deserializing such a property assigns a struct array of references to it,
which MATLAB converts by calling the controlled term constructor with the
whole array. initializeControlledTerm expands obj to one element per
struct, but it is a method on a handle class that returned nothing, so the
expansion applied only to the local copy. The constructor kept the
original scalar handle, which is the first element, and the rest were
discarded.

The method now returns the object array and the three constructors that
call it take the returned value.

This takes the round-trip suite from 261 to 288 of 292 types, and removes
30 entries from the known-gap list.

The four remaining failures are unrelated to each other and are now
listed individually: two types fail because serializing an unresolved
MixedTypeReference is rejected by the meta type registry, ChemicalSubstance
fails because a controlled instance whose name is not a valid MATLAB
identifier cannot be looked up from a bare reference, and TermSuggestion
fails because of the missing LINKED_PROPERTIES entry described in the
previous PR.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@ehennestad
ehennestad force-pushed the fix-controlled-term-array-construction branch from 8a6c8c9 to 818b5de Compare September 1, 2026 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant