aws-cqc / aws-cqc/DeviceLayout.jl

ParameterSetYAMLExt: save_parameter_set output not consumable by ParameterSet(path) for Vector/Point/Tuple Unitful leaves (round-trip asymmetry)

Closed
#253 2 comments 0 reactions 0 assignees View on GitHub
bug
Dominant language
Julia
Stars
67
Forks
14
Avg merge
1d 19h
Merged PRs (30d)
24

Description

## Summary

`ParameterSetYAMLExt` does not satisfy the self-round-trip invariant

```julia
ParameterSet(save_parameter_set(ps)) == ps
```

for leaf values that are **`Vector`/`Tuple`/`Point` of `Unitful.Quantity`**. It holds for *scalar*
Unitful leaves, but a `ParameterSet` written by `save_parameter_set` and read back by
`ParameterSet(path)` comes back with those collection leaves as **`String` / `Vector{String}`**
instead of quantities — so the library's own output is not consumable by its own loader.

**Classification:** the core report (collections of `Quantity` above — the ★ ask) is a **bug**: the
writer emits values its own reader silently mis-parses, and the two paths aren't even format-consistent
(scalars are written space-less `"150.0μm"`, which `uparse` accepts; elements are written
space-separated `383.0 nm`, which `uparse` rejects). The fix is inherently backward-compatible —
scalar leaves already round-trip and can't regress. The `Point`/`Tuple` *type-preservation* and
strict-load items further down are **enhancements** (they add capability the library doesn't have
today), flagged separately so the bug fix isn't gated on them.

**Version:** DeviceLayout **1.15.0** (`ext/ParameterSetYAMLExt.jl`). Not blocking us — we have a
consumer-side workaround — but it makes a saved `ParameterSet` YAML unsafe to use as a *load* source
without extra code, which defeats "generate the YAML, then build from it."

## Minimal reproducer (library-only; verified on 1.15.0)

```julia
using DeviceLayout
using DeviceLayout.PreferredUnits # μm, nm, Point
using DeviceLayout.SchematicDrivenLayout: ParameterSet, save_parameter_set
using Unitful
using YAML # activates ParameterSetYAMLExt

ps = ParameterSet()
ps.components.demo.scalar_q = 150.0μm # scalar Quantity (control)
ps.components.demo.plain = 4 # plain Int (control)
ps.components.demo.vec_q = [383.0, 393.0]u"nm" # Vector{Quantity}
ps.components.demo.point = Point(4.0μm, -8.318μm) # Point
ps.components.demo.vec_pt = [Point(0.0μm, -5.5μm), Point(4.0μm, -8.318μm)] # Vector{Point}
ps.components.demo.tup = (50μm, 35μm, 50μm) # Tuple{Quantity}

mktemp() do path, io
close(io)
save_parameter_set(path, ps)
loaded = ParameterSet(path)
for k in (:scalar_q, :plain, :vec_q, :point, :vec_pt, :tup)
before = getproperty(ps.components.demo, k)
after = getproperty(loaded.components.demo, k)
println(rpad(k, 10), rpad(typeof(before), 34), " → ", rpad(typeof(after), 24),
isequal(before, after) ? " ✅ ==" : " ❌ NOT ==")
end
end
```

### Observed output (1.15.0)

```
scalar_q Quantity{Float64,...ContextUnits...} → Quantity{...} ✅ ==
plain Int64 → Int64 ✅ ==
vec_q Vector{Quantity{...}} → Vector{String} ❌ NOT ==
point Point{Quantity{...}} → Vector{String} ❌ NOT ==
vec_pt Vector{Point{Quantity{...}}} → Vector{Vector{String}} ❌ NOT ==
tup Tuple{Quantity,Quantity,Quantity} → String ❌ NOT ==
```

The serialized YAML shows the asymmetry directly:

```yaml
components:
demo:
scalar_q: "150.0μm" # scalar: quoted, space-LESS → uparse re-parses on load ✅
vec_q:
- 383.0 nm # element: unquoted, SPACE → never re-parsed on load ❌
point:
- 4.0 μm # Point: anonymous 2-list → can't tell it's a Point ❌
- -8.318 μm
tup: (50 μm, 35 μm, 50 μm) # Tuple: opaque show-string → not even a sequence ❌
```

## Root cause (`ext/ParameterSetYAMLExt.jl`)

Two symmetric gaps:

- **`_serialize_units`** stringifies a value only when it `isa Unitful.Quantity`. A `Vector`/`Tuple`
of quantities is not itself a `Quantity`, so it falls through to YAML's default rendering (bare
`383.0 nm` tokens; a `Tuple` renders via `show` as the string `"(50 μm, …)"`).
- **`_parse_units!`** `uparse`s a value only when it `isa AbstractString` inside a `Dict`; it never
recurses into `AbstractArray`/`Tuple`, so collection elements are never converted back on load.

Plus a **format mismatch**: scalars are written space-less (`"150.0μm"`, which `uparse` accepts) but
collection elements are written space-separated (`383.0 nm`, which `uparse` rejects).

## Suggested fixes (tiered — any subset helps; the first is the ask)

**★ Minimum (the actual request): recurse into collections.**
Make `_serialize_units` **and** `_parse_units!` recurse through `AbstractArray`/`Tuple`, and emit the
same space-less element format the scalar path uses (or make the element parse tolerate the space).
This fixes every `Vector{Quantity}` leaf directly. For `Tuple{Quantity}` there's an extra half-step:
today it isn't even written as a YAML sequence (it goes out as the `show`-string `(50 μm, …)`), so the
serializer must first emit tuples as sequences — then the same recursion covers them (a `Tuple` will
round-trip back as a `Vector`, which is fine for splat-style consumption; exact `Tuple` *type* identity
is the ◆ item). Please route elements through the *same* `uparse` the scalar path uses so unit context
matches — note in the repro that the scalar comes back as `ContextUnits`, so elements should too (a
hand-rolled element parse would yield bare `FreeUnits`).

**◆ If willing: preserve the exact Julia TYPE of `Point` / `Tuple` leaves.**
This is the genuine *enhancement* part (the ★ fix restores the values as generic sequences; this
restores the concrete type). `Point` and `Tuple` both come back as `Vector` after ★ — fine when the
consumer splats them, but not a faithful type round-trip. Closing that needs a typed encoding the
loader can key on — e.g. a YAML tag (`!point [x, y]`) or a `{__type__: Point, data: [x, y]}` envelope
on save/load, **or** a registerable `serialize_leaf(::T)` / `deserialize_leaf(::Type{T}, node)` hook so
downstreams can add types without patching the ext. (If you'd rather not, just **documenting** that
`Point`/`Tuple` round-trip as `Vector` is enough — we'll reconstruct the type on our side.)

**○ Nice-to-have: a strict load + a round-trip test.**
- `ParameterSet(path; strict=true)` (or `validate(ps, T)`) that **errors** when a leaf can't be
coerced to the expected type instead of silently leaving a `String` — the failure here is *silent*
(a valid-looking PS that builds the wrong geometry), so a loud error would catch it immediately.
- A `roundtrip(ps)` helper + `@test ParameterSet(save_parameter_set(ps)) == ps` in CI would pin the
invariant. (The repro above is essentially that test.)

Contributor guide

Open the contributing guide

Research direction

Start in ext/ParameterSetYAMLExt.jl by reading _serialize_units and _parse_units! and run the minimal reproducer from the issue. Verify that Vector, Point, and Tuple leaves containing Unitful quantities are serialized and loaded as usable sequences rather than strings, while scalar quantities still round-trip. Add or run a regression test covering the round-trip invariant and confirm the existing scalar behavior remains unchanged.

Written by the indexing model from the issue text.

Assessment

Tech stack
julia, yaml
Domain
testing, tooling
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
72/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.