Look up IDS metadata by type, not by location string - #108
Conversation
`units(::IDS, ::Symbol)` routed through the universal-location string, which resolves the struct name inside IMASdd. Any IDS owned by a satellite data dictionary threw `UndefVarError` even though it is registered in `_all_info`. `show` calls units per field, so displaying a satellite container failed. Look the field up by type through the existing `info(::IDS, ::Symbol)` instead.
`cocos_transform(::IDS, ::Symbol)` and the strict branch of `get_frozen_strict_property` built a universal-location string and resolved the struct name back inside IMASdd, the same round-trip already removed from `units`. Satellite-owned IDSs threw `UndefVarError` there, which made `imas2json(dd; strict=true)` fail outright for a satellite container. `_all_info` is keyed by `(Type, Symbol)`, so the type in hand is the exact key; the string is a lossy projection of it — two containers can share one location. Verified across a full dd walk (28,181 fields, IDSvector elements materialized): units, .extra and cocos_transform are identical to the string route everywhere, and the returned `Info` objects are `===`. Strict export also gets cheaper: 623 to 569 us, 337 fewer allocations. The satellite fixture now registers `global_time` on its own container, as a generated satellite does — previously it leaned on the silent fallback to IMASdd.dd that this change removes.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #108 +/- ##
=======================================
Coverage 43.92% 43.93%
=======================================
Files 16 16
Lines 31551 31551
=======================================
+ Hits 13859 13862 +3
+ Misses 17692 17689 -3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@mgyoo86 Why does it say that "The round-trip works inside IMASdd, but it is slower, allocates more", but the benchmarks are showing faster times with less allocations? |
This describes master's behavior that uses This PR changes the behavior by directly using the target ids (already given as a function arg), avoiding the unneccessary cost for the temporary string stuff. The benchmark is showing the performance difference between those two implementations. You can consider this PR a performance improvement PR for IMASdd. |
2 participants
Summary
A few functions took an IDS, built a universal-location string from it, and then resolved that string back to a type to look up
_all_info. The round-trip works inside IMASdd, but it is slower, allocates more, and the string does not identify a type —IMASdd.summary__codeandIFEdd.summary__codeboth map tosummary.code, so satellite-owned IDSs either threwUndefVarErroror silently resolved against IMASdd's entry._all_infois keyed by(Type, Symbol). When the type is already in hand it is the exact key, so these now look it up directly — safer and faster.Changes
units(::IDS, ::Symbol)→info(ids, field).unitscocos_transform(::IDS, ::Symbol)→info(ids, field).cocos_transformget_frozen_strict_property, both methods →info(ids, field).extraunits(::String)andcocos_transform(::String)are unchanged — a location string is still the right input when no type is available.Displaying a satellite container, and
imas2json(dd; strict=true)on one, now work instead of throwing. Verified against the old route across a fullIMASdd.dd()walk: identical results everywhere.Benchmark
imas2json(strict=true)