fix: round trip of a Solution with unset optional solver fields

CHANGELOG.md

@@ -50,6 +50,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   - `Solution` gains an eleventh field and type parameter, `switching`; the ten-argument
     positional constructor is kept and builds an `EmptySwitchingModel`.
 
+### 🔧 Fixed
+
+- **Export/import round trip of a `Solution` with unset optional solver fields**
+  ([#464](https://github.com/control-toolbox/CTModels.jl/issues/464)). A solution whose
+  `iterations`, `constraints_violation`, `message` or `status` are `NotProvided` (as the
+  ones built by an indirect method) could be exported but not imported, in JSON and JLD2.
+  An unset field is now written as `null` (JSON) / `nothing` (JLD2) and read back as
+  `NotProvided`; files holding the string `"NotProvided"` (older versions) are still read.
+  A value of the wrong type raises a `ParsingError` instead of a `MethodError`.
+
 ### 📚 Documentation
 
 - New page *Switching structure*; the `build_solution` contract, plotting and

ext/CTModelsJLD.jl

@@ -84,7 +84,13 @@ julia> using JLD2, CTModels
 julia> sol = CTModels.import_ocp_solution(CTModels.JLD2Tag(), model; filename="mysolution")
 ```
 
+# Throws
+- `CTBase.Exceptions.ParsingError`: if an optional solver field has a value of the wrong type.
+
 # Notes
+- An unset optional solver field (`iterations`, `constraints_violation`, `message`, `status`)
+  is stored as `nothing` and read back as `Core.NotProvided` (`Core.NotProvided` stored by
+  older versions is read the same way)
 - The solution is reconstructed from discretized data via `build_solution`
 - This ensures perfect round-trip consistency with the export
 - The file does not contain the model: `ocp` must be the model the solution was computed

ext/CTModelsJSON.jl

@@ -296,8 +296,13 @@ including the discretized primal and dual trajectories.
 - `CTBase.Exceptions.IncorrectArgument`: if the file does not match `ocp` (dimensions,
   number or labels of constraints, fixed versus free times). Pass the model the solution
   was computed from.
+- `CTBase.Exceptions.ParsingError`: if an optional solver field has a value of the wrong type.
 
 # Notes
+An unset optional solver field (`iterations`, `constraints_violation`, `message`, `status`)
+is written as `null` and read back as `Core.NotProvided`; the string `"NotProvided"`, written
+by older versions, is read the same way.
+
 Handles both vector and matrix encodings of signals. If dual fields are missing or `null`, the corresponding attributes are set to `nothing`.
 
 The file is checked against `ocp` before the solution is rebuilt; see
@@ -332,10 +337,10 @@ function CTModels.import_ocp_solution(
 
     data = Dict{String,Any}(
         "objective" => blob["objective"],
-        "iterations" => blob["iterations"],
-        "constraints_violation" => blob["constraints_violation"],
-        "message" => blob["message"],
-        "status" => blob["status"],
+        "iterations" => get(blob, "iterations", nothing),
+        "constraints_violation" => get(blob, "constraints_violation", nothing),
+        "message" => get(blob, "message", nothing),
+        "status" => get(blob, "status", nothing),
         "successful" => blob["successful"],
         "state" => _json_to_matrix(blob["state"]),
         "control" => _json_to_matrix(blob["control"]),

src/Serialization/reconstruction_helpers.jl

@@ -4,6 +4,34 @@
 """
 $(TYPEDSIGNATURES)
 
+Read the optional solver field `key` of `data` as a `T`, or `Core.NotProvided` if it is unset.
+
+A field is unset when its key is missing or when its value is `nothing` (`null` in JSON),
+`Core.NotProvided`, or the string `"NotProvided"` (written by older versions).
+
+# Throws
+- `CTBase.Exceptions.ParsingError`: if the value cannot be converted to `T`.
+"""
+function _optional(data, key::String, ::Type{T}) where {T}
+    value = get(data, key, nothing)
+    (isnothing(value) || value isa CTBase.Core.NotProvidedType || value == "NotProvided") &&
+        return CTBase.Core.NotProvided
+    try
+        return T(value)
+    catch
+        throw(
+            Exceptions.ParsingError(
+                "solution field '$key' must be a $T, got $(repr(value))";
+                location="imported solution dictionary",
+                suggestion="re-export the solution with a current CTModels version",
+            ),
+        )
+    end
+end
+
+"""
+$(TYPEDSIGNATURES)
+
 Reconstruct a solution from imported data, detecting the format (single vs multiple time grids).
 
 Duals and `control_interpolation` are read from `data`. Only `infos` is accepted as a
@@ -73,10 +101,10 @@ function _reconstruct_solution_from_data(ocp, data; infos=Dict{Symbol,Any}())
             _extract_time_vector(data["variable"]),
             data["costate"];
             objective=Float64(data["objective"]),
-            iterations=data["iterations"],
-            constraints_violation=Float64(data["constraints_violation"]),
-            message=data["message"],
-            status=Symbol(data["status"]),
+            iterations=_optional(data, "iterations", Int),
+            constraints_violation=_optional(data, "constraints_violation", Float64),
+            message=_optional(data, "message", String),
+            status=_optional(data, "status", Symbol),
             successful=data["successful"],
             path_constraints_dual=path_constraints_dual,
             boundary_constraints_dual=boundary_constraints_dual,
@@ -111,10 +139,10 @@ function _reconstruct_solution_from_data(ocp, data; infos=Dict{Symbol,Any}())
             _extract_time_vector(data["variable"]),
             data["costate"];
             objective=Float64(data["objective"]),
-            iterations=data["iterations"],
-            constraints_violation=Float64(data["constraints_violation"]),
-            message=data["message"],
-            status=Symbol(data["status"]),
+            iterations=_optional(data, "iterations", Int),
+            constraints_violation=_optional(data, "constraints_violation", Float64),
+            message=_optional(data, "message", String),
+            status=_optional(data, "status", Symbol),
             successful=data["successful"],
             path_constraints_dual=path_constraints_dual,
             boundary_constraints_dual=boundary_constraints_dual,

src/Solutions/build_solution.jl

@@ -1728,6 +1728,15 @@ const SERIALIZATION_FORMAT_VERSION = 2
 """
 $(TYPEDSIGNATURES)
 
+Return `nothing` for an unset optional field (`Core.NotProvided`), and `x` otherwise.
+
+An unset field is written as `nothing` (`null` in JSON), whatever the format.
+"""
+_unset_to_nothing(x) = x isa Core.NotProvidedType ? nothing : x
+
+"""
+$(TYPEDSIGNATURES)
+
 Return the key of the serialized `side` limit (`1` left, `2` right) of `component`, e.g.
 `"switching_state_left"`.
 """
@@ -1845,7 +1854,9 @@ Dict(
     - `"control_constraints_lb_dual"`, `"control_constraints_ub_dual"`: Control box duals on control grid
     - `"boundary_constraints_dual"`: Boundary duals (time-independent vector)
     - `"variable_constraints_lb_dual"`, `"variable_constraints_ub_dual"`: Variable duals (vectors)
-  - **Solver info**: `"iterations"`, `"message"`, `"status"`, `"successful"`, `"constraints_violation"`, `"infos"`
+  - **Solver info**: `"iterations"`, `"message"`, `"status"`, `"successful"`, `"constraints_violation"`, `"infos"`.
+    The optional fields `"iterations"`, `"message"`, `"status"` and `"constraints_violation"` are
+    `nothing` when unset (`Core.NotProvided`)
   - **Compatibility check**: `"format_version"` and `"model_signature"` (see
     [`CTModels.Models._model_signature`](@extref)), used at import time to check the file against the model
 
@@ -2005,11 +2016,11 @@ function _discretize_all_components(
         "boundary_constraints_dual" => boundary_constraints_dual(sol),
         "variable_constraints_lb_dual" => _as_vector(variable_constraints_lb_dual(sol)),
         "variable_constraints_ub_dual" => _as_vector(variable_constraints_ub_dual(sol)),
-        "iterations" => iterations(sol),
-        "message" => message(sol),
-        "status" => status(sol),
+        "iterations" => _unset_to_nothing(iterations(sol)),
+        "message" => _unset_to_nothing(message(sol)),
+        "status" => _unset_to_nothing(status(sol)),
         "successful" => successful(sol),
-        "constraints_violation" => constraints_violation(sol),
+        "constraints_violation" => _unset_to_nothing(constraints_violation(sol)),
         "infos" => infos(sol),
         "format_version" => SERIALIZATION_FORMAT_VERSION,
         "model_signature" => Models._model_signature(model(sol)),

test/suite/serialization/test_optional_fields.jl

@@ -0,0 +1,143 @@
+module TestOptionalFields
+
+using Test: Test
+using JSON: JSON
+using JLD2: JLD2
+using CTBase: CTBase
+using CTModels: Building
+using CTModels: Solutions
+using CTModels: Serialization
+
+const VERBOSE = isdefined(Main, :TestData) ? Main.TestData.VERBOSE : true
+const SHOWTIMING = isdefined(Main, :TestData) ? Main.TestData.SHOWTIMING : true
+
+const NP = CTBase.Core.NotProvided
+
+const FORMATS = (:JSON, :JLD)
+const FIELDS = (:iterations, :constraints_violation, :message, :status)
+
+function _ocp()
+    pre = Building.PreModel()
+    Building.time_dependence!(pre; autonomous=true)
+    Building.time!(pre; t0=0.0, tf=1.0)
+    Building.state!(pre, 1)
+    Building.control!(pre, 1)
+    Building.dynamics!(pre, (r, t, x, u, v) -> (r[1] = u; nothing))
+    Building.objective!(pre, :min; lagrange=(t, x, u, v) -> 0.5 * u^2)
+    return Building.build(pre)
+end
+
+function _solution(ocp; set::Bool)
+    T = collect(range(0.0, 1.0, 11))
+    extra = if set
+        (; iterations=7, constraints_violation=1e-9, message="converged", status=:Success)
+    else
+        (;)
+    end
+    return Solutions.build_solution(
+        ocp, T, t -> [t], t -> [1.0], Float64[], t -> [0.0];
+        objective=0.5, successful=true, extra...,
+    )
+end
+
+_values(sol) = map(f -> getfield(Solutions, f)(sol), FIELDS)
+
+_ext(fmt) = fmt === :JSON ? ".json" : ".jld2"
+
+# What the previous version wrote for an unset field.
+function _make_legacy(fmt, path)
+    if fmt === :JSON
+        blob = JSON.parse(read(path, String); dicttype=Dict{String,Any})
+        for k in String.(FIELDS)
+            blob[k] = "NotProvided"
+        end
+        write(path, JSON.json(blob; pretty=true))
+    else
+        data = JLD2.load(path)["solution_data"]
+        data["iterations"] = NP
+        data["constraints_violation"] = NP
+        data["message"] = NP
+        data["status"] = NP
+        JLD2.jldsave(path; solution_data=data)
+    end
+    return nothing
+end
+
+function _import_with(ocp, fmt, path)
+    return Serialization.import_ocp_solution(ocp; format=fmt, filename=path)
+end
+
+function test_optional_fields()
+    Test.@testset "Optional solver fields round trip (#464)" verbose = VERBOSE showtiming =
+        SHOWTIMING begin
+        ocp = _ocp()
+        mktempdir() do dir
+            for fmt in FORMATS
+                Test.@testset "$fmt" begin
+                    path = joinpath(dir, "sol_$(fmt)")
+                    file = path * _ext(fmt)
+
+                    Test.@testset "all four fields unset" begin
+                        sol = _solution(ocp; set=false)
+                        Test.@test all(v -> v === NP, _values(sol))
+                        Serialization.export_ocp_solution(sol; format=fmt, filename=path)
+                        back = _import_with(ocp, fmt, path)
+                        Test.@test all(v -> v === NP, _values(back))
+                        Test.@test Solutions.successful(back)
+                    end
+
+                    Test.@testset "all four fields set" begin
+                        sol = _solution(ocp; set=true)
+                        Serialization.export_ocp_solution(sol; format=fmt, filename=path)
+                        back = _import_with(ocp, fmt, path)
+                        Test.@test Solutions.iterations(back) == 7
+                        Test.@test Solutions.constraints_violation(back) == 1e-9
+                        Test.@test Solutions.message(back) == "converged"
+                        Test.@test Solutions.status(back) === :Success
+                    end
+
+                    Test.@testset "legacy file with \"NotProvided\"" begin
+                        sol = _solution(ocp; set=false)
+                        Serialization.export_ocp_solution(sol; format=fmt, filename=path)
+                        _make_legacy(fmt, file)
+                        back = _import_with(ocp, fmt, path)
+                        Test.@test all(v -> v === NP, _values(back))
+                    end
+
+                    Test.@testset "wrong type raises a ParsingError" begin
+                        sol = _solution(ocp; set=true)
+                        Serialization.export_ocp_solution(sol; format=fmt, filename=path)
+                        if fmt === :JSON
+                            blob = JSON.parse(read(file, String); dicttype=Dict{String,Any})
+                            blob["iterations"] = "many"
+                            write(file, JSON.json(blob; pretty=true))
+                        else
+                            data = JLD2.load(file)["solution_data"]
+                            data["iterations"] = "many"
+                            JLD2.jldsave(file; solution_data=data)
+                        end
+                        Test.@test_throws CTBase.Exceptions.ParsingError _import_with(
+                            ocp, fmt, path
+                        )
+                    end
+                end
+            end
+
+            Test.@testset "JSON file holds null, not \"NotProvided\"" begin
+                path = joinpath(dir, "null_check")
+                Serialization.export_ocp_solution(
+                    _solution(ocp; set=false); format=:JSON, filename=path
+                )
+                blob = JSON.parse(read(path * ".json", String); dicttype=Dict{String,Any})
+                for k in String.(FIELDS)
+                    Test.@test haskey(blob, k) && blob[k] === nothing
+                end
+            end
+        end
+    end
+    return nothing
+end
+
+end # module TestOptionalFields
+
+test_optional_fields() = TestOptionalFields.test_optional_fields()