sow/2026-Q2 SAM#377
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 99e497947a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…asshopper component implemented
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ee3815f6d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| { | ||
| adjacencyCluster = new AdjacencyCluster(adjacencyCluster, true); | ||
|
|
||
| mechanicalSystems = adjacencyCluster.SplitSystemsByZones<VentilationSystem>(zoneCategoryName, addPrefix); |
There was a problem hiding this comment.
SAMAnalyticalModifyMechanicalSystemsByZone is wired to split only VentilationSystem objects, so heating and cooling systems that span multiple zones remain unsplit even though the component is named and exposed as modifying mechanical systems broadly. In projects where zone-category partitioning is expected for all system categories, downstream reports and assignments stay inconsistent for heating/cooling.
Useful? React with 👍 / 👎.
…difyVentilationSystemsByZone
Drop the Newtonsoft.Json package dependency from SAM.Core, SAM.Geometry, SAM.Math, SAM.Weather, SAM.Analytical, SAM.Architectural, SAM.Units and their Grasshopper/Rhino UI projects. The Newtonsoft types JObject, JArray, JToken, JValue, JProperty, JsonConvert, JsonSerializerSettings, Formatting, NullValueHandling and JTokenType are reintroduced in the new namespace SAM.Core.Json as a thin compatibility shim over System.Text.Json.Nodes. Source files swap their using directive from Newtonsoft.Json.Linq to SAM.Core.Json with no other changes. The shim's JTokenType getter, ToValue and ToNode are tightened to match the round-trip behaviour the SAM codebase relies on: - The Guid string-parse heuristic is removed because downstream switches (ParameterSet, RelationCluster, ParameterFilter) have no Guid case and would silently drop parameters whose value is a Guid string. - The Date string-parse heuristic is restricted to strings whose shape matches strict ISO 8601 date-time (YYYY-MM-DDTHH:MM:SS...), so names, labels and version tags no longer get reclassified as DateTime. - ToValue tries TryGetValue<string> before TryGetValue<Guid>/<DateTime> so JSON string values surface as strings unless explicitly typed. - ToNode emits doubles, floats and decimals with an explicit decimal point via JsonNode.Parse so that 5.0 survives the round trip as Float instead of being silently demoted to Integer by System.Text.Json's default trailing-zero trimming. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The three accessors return the result on lookup failure and the default sentinel on success — branches were inverted versus their sibling accessors (ToString, ToDouble, ToInt, ToBool, ToColor). Effect: any caller asking for a DateTime/JObject/JArray parameter always received DateTime.MinValue or null regardless of whether the value was present. Pre-existing bug, predates the System.Text.Json migration; surfaced by the new ParameterSet round-trip test. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
xUnit project targeting net8.0 that exercises SAM JSON serialization through the public IJSAMObject surface only — no JObject type is named, so the same source runs unchanged against Newtonsoft and against the System.Text.Json shim. JSON equivalence is structural via System.Text.Json.JsonDocument (order-insensitive on object keys, order-sensitive on arrays, whitespace-independent). Coverage: - ParameterSetTests — one test per JTokenType branch in ParameterSet.FromJObject (String, Integer, fractional Double, whole-number Double, Boolean, DateTime, Guid, Color, mixed) plus a heuristic guard for strings that look like dates. - SAMObjectTests — Guid/Name preservation and two-pass idempotency. - LibraryFixtureTests — every SAM_*Library.JSON under files/resources/Analytical/ is loaded, deserialized, re-serialized and structurally compared. Current state: 26/28 pass. The remaining 2 failures (StringThatLooksLikeDate, InternalConditionLibrary non-idempotency on LightingOccupancyControls) are pre-existing on master and not introduced by the migration. Not added to SAM.sln; build and run from SAM/SAM.Tests directly with dotnet test until it's wired into the main solution. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Aligns the repo with the SAM-BIM workspace-wide pin to the LTS-aligned System.Text.Json 8.0.5, replacing the previously pinned 10.0.8 preview.
Aligns the repo with the SAM-BIM workspace-wide pin to the LTS-aligned System.Text.Json 8.0.5, replacing the previously pinned 10.0.8 preview.
Pin System.Text.Json to 8.0.5
…#5) Eliminates CS1607 warnings and restores deterministic builds. Aligns with the workspace's existing fixed-version files. Scope: AssemblyInfo.cs + .csproj version attributes only. The // commented-out template example is left intact for documentation.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c4dd9a85d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (string.IsNullOrWhiteSpace(json)) | ||
| return null; | ||
|
|
||
| JsonNode jsonNode = JsonNode.Parse(json); |
There was a problem hiding this comment.
JsonNode.Parse(json) now runs without a guard, so any non-JSON string throws instead of failing gracefully. This is reachable from Query.TryConvert's IJSAMObject branch (it calls Convert.ToSAM before SAMColor fallback parsing), which means inputs like non-JSON color strings can now raise exceptions and abort conversion paths that previously returned null/false.
Useful? React with 👍 / 👎.
| JsonArray jsonArray = jsonNode as JsonArray; | ||
| if (jsonArray == null) | ||
| { | ||
| return null; |
There was a problem hiding this comment.
The string overload now accepts only JsonArray and returns null for a valid single-object payload. The previous behavior wrapped a JSON object into a one-item array, so callers could deserialize either {...} or [{...}]; with this change, object-form inputs silently stop deserializing.
Useful? React with 👍 / 👎.
…nvert.SerializerOptions - ExportHydra.cs: switch JsonSerializerOptions to Core.Convert.SerializerOptions(Core.Formatting.Indented) - Apertures.cs: new perf path (~80 lines added) - IntersectionDictionary.cs / ViewField.cs / VisibleLinkedFace3Ds.cs: spatial-query perf improvements - File.cs / String.cs: small Convert-namespace refactors
Perf: spatial Query refactor + JSON serializer options
* build: introduce Directory.Build.props for centralised version stamping Single $(SAMVersion) property at repo root replaces literal AssemblyVersion/ FileVersion duplicated across csproj files and AssemblyInfo.cs files. - Directory.Build.props at SAM root: defaults SAMVersion to 1.0.0.0, sets AssemblyVersion/FileVersion from it, sets Deterministic=true. - For projects with GenerateAssemblyInfo=false, a Target writes obj/SAMVersion.g.cs before CoreCompile so the attributes also reach those assemblies. - Strips now-duplicated <AssemblyVersion>/<FileVersion>/<Deterministic>false lines from 13 csproj files. - Strips [assembly: AssemblyVersion]/[assembly: AssemblyFileVersion] from 6 Grasshopper AssemblyInfo.cs files. AssemblyTitle/Guid/etc are preserved. - build.yml computes SAMVersion from the sow/yyyy-Qx branch name and passes /p:SAMVersion=YYYY.Q.<run_number> to msbuild. Local dev builds remain at 1.0.0.0. Side effect: SAM.Units.dll, which had <GenerateAssemblyInfo>false</GenerateAssemblyInfo> but no Properties/AssemblyInfo.cs and was therefore shipping as 0.0.0.0, now stamps correctly via the generated source file. Verified locally with /p:SAMVersion=2026.2.999: SAM.Core, SAM.Math, SAM.Units, SAM.Core.Grasshopper, SAM.Math.Grasshopper all report FileVersion=2026.2.999. * ci: trigger build on sow/** + date fallback for SAMVersion Codex review caught that build.yml only triggered on master/main, making the sow/yyyy-Qx regex branch unreachable. Two fixes: 1. Add 'sow/**' to push and pull_request branch filters so PRs and pushes against sow/2026-Q2 (and future quarter branches) actually run the build job, not just SPDX. 2. When the ref doesn't match sow/yyyy-Qx (e.g. on master/main pushes, feature branch workflow_dispatch), derive year+quarter from current UTC date instead of falling back to 1.0.0.0. This means every CI build stamps a real version, not just sow/* builds. Examples: - push to master in May 2026 -> SAMVersion=2026.2.<run> (from date) - PR to sow/2026-Q2 -> SAMVersion=2026.2.<run> (from branch) - workflow_dispatch on any -> SAMVersion=YYYY.Q.<run> (date or branch) * ci: cap run_number at 60000 (% modulo) to stay within UInt16 limit Codex P2: .NET AssemblyVersion components are UInt16 (max 65535). github.run_number is monotonic and grows without bound, so unbounded use would eventually break msbuild on `/p:SAMVersion=...`. Currently run_number=14, so cap is a no-op for ~65k more builds. After that the third octet wraps to 0 — not ideal but year+quarter still keep the version uniquely identifiable across wraps.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1adf1ca4db
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Value = Core.Create.IJSAMObject<Transform3D>(value); | ||
| return true; |
There was a problem hiding this comment.
Read now assigns Value = Core.Create.IJSAMObject<Transform3D>(value) and immediately returns true even when the parse does not produce a Transform3D (for example, malformed/legacy JSON or missing _type). In that case the goo reports successful deserialization but keeps a null payload, which silently drops persisted data and defers failure to later component execution instead of failing at read time.
Useful? React with 👍 / 👎.
- Append .0 to make SAMVersion a 4-part dotted version. AssemblyFileVersion expects 4 parts; 3 parts produces compiler warnings and inconsistent Windows file-version metadata. - For PR events, prefer github.head_ref when it matches sow/yyyy-Qx. This matters for release-promotion PRs (sow/2026-Q2 -> main) where base_ref is main but the meaningful version source is the PR's source branch. - Existing behaviour preserved for the common case (PR feature->sow/2026-Q2).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d87d98e41
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| IJSAMObject result = Activator.CreateInstance(type) as IJSAMObject; | ||
| if (result != null && result.FromJsonObject(jsonObject)) |
There was a problem hiding this comment.
The new fallback path unconditionally calls Activator.CreateInstance(type) when no JsonObject constructor exists, which throws for valid SAM types that have no parameterless constructor (for example TM59Result and related result classes under SAM.Analytical/Classes/Result/TM). Before this change those payloads were safely wrapped; now deserializing the same JSON can raise MissingMethodException and abort model/result loading.
Useful? React with 👍 / 👎.
| @@ -50,17 +46,15 @@ public static List<T> IJSAMObjects<T>(this string json) where T : IJSAMObject | |||
| return default; | |||
| } | |||
|
|
|||
| // Convert the JSON string to a JArray | |||
| JArray jArray = Query.JArray(json); | |||
| JsonNode jsonNode = JsonNode.Parse(json); | |||
There was a problem hiding this comment.
This overload now calls JsonNode.Parse(json) directly, so malformed or non-JSON input throws instead of returning null like the previous implementation (Query.JArray path). Callers that treat parse failures as non-exceptional (e.g., probing conversion paths) will now crash rather than gracefully failing.
Useful? React with 👍 / 👎.
Catches classic (non-SDK) csprojs that don't set GenerateAssemblyInfo at all (evaluates as empty string). The previous '== false' condition skipped them, leaving their DLLs at compiler defaults. This fix was synced to all 30 cascade repos during the Stage 2 sweep but never committed back to SAM itself — SAM's working tree had the fix but the remote sow/2026-Q2 didn't, so SAM_Deploy installer builds were cloning SAM with the stale condition.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Path D's SAMVersion.g.cs Target now also emits AssemblyInformationalVersionAttribute when CI provides $(InformationalVersion) — covers GenerateAssemblyInfo=false projects (Grasshopper components, SAM_Tas bridge assemblies) which SDK auto-gen doesn't touch. For SDK projects, PropertyGroup composes InformationalVersion from $(SAMVersion) + $(SAMSourceRevision) when CI sets the latter; SDK auto-gen handles the attribute. Both mechanisms work side by side. Local dev builds (no CI env vars set) leave InformationalVersion empty and skip emitting the attribute — unchanged behaviour for developers.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd02a4c266
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| JsonObject dataObject = new JsonObject | ||
| { | ||
| ["Name"] = keyValuePair.Key, | ||
| ["Value"] = keyValuePair.Value |
There was a problem hiding this comment.
FromJsonObject keeps missing hourly measurements as double.NaN, but ToJsonObject writes keyValuePair.Value directly into a JsonObject. With the new System.Text.Json path, serializing that node (for example via Convert.ToString(...) / ToJsonString) throws on non-finite numbers, so weather datasets that contain missing values can no longer round-trip and will fail at export/save time.
Useful? React with 👍 / 👎.
The .NET SDK's SourceLink integration appends the project's own commit SHA to InformationalVersion when SourceLink is configured (e.g. via NuGet). That produces inconsistent ProductVersion strings across our DLLs — most show '2026.2.164.0+998af06' (our explicit SAM_Deploy SHA) but SourceLink-configured ones show '2026.2.164.0+998af06.<long-project-sha>'. Setting IncludeSourceRevisionInInformationalVersion=false tells the SDK to keep whatever we explicitly set, no auto-append. All DLLs now show the same +SHA suffix (SAM_Deploy commit) which is what we want — that single SHA encodes all 22 submodule pointers via gitlinks, the full content fingerprint.
…nce field to LinkedFace3D - AnalyticalModelParameter.SolarModel — new IJSAMObject parameter slot shared by the TAS-import path and the SAM solar engine so downstream comparison nodes are provenance-agnostic - LinkedFace3D — adds optional Reference string (new ctor overload + JSON round-trip) for storing an external reference key such as a TBD zoneSurface.GUID Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…operativeTemperatures (#9) `MaxExceedableHours` and `GetHoursNumberExceeding28` both consume the `OperativeTemperatures` IndexedDoubles via `GetMaxIndex().Value` / `GetMinIndex().Value`. When the underlying collection is empty (or sparse enough that no max/min index can be derived) those `.Value` accesses on a null `Nullable<int>` throw `InvalidOperationException`, which surfaces during overheating analysis whenever a corridor result row comes back with no operative-temperature samples. Add defensive guards mirroring the existing `operativeTemperatures == null` early-return: * `operativeTemperatures.Count <= 0` → return 0 * `GetMaxIndex()` or `GetMinIndex()` is null → return 0 `MaxExceedableHours` now uses the locals from the pattern match directly (`int count = maxIndex - minIndex + 1;`) instead of re-calling the API, which also drops two redundant getter invocations on the hot path. `GetHoursNumberExceeding28` matches the same guard shape for consistency even though the body itself only iterates and doesn't dereference the indices — keeps the two methods aligned and prevents a future edit from introducing a regression. No behavioural change for callers passing well-formed results; previously crashing inputs now return 0 (matching the existing -1/null-return convention for "no useful answer" in this class). Companion to SAM_Tas PR #10 (Stage 2 perf pass) — surfaced while running the same overheating workflow used to QA that branch. The two PRs are independent code-wise but intended to ship in the same QA round. Local Framework MSBuild Release on SAM.Analytical: 0 errors. Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Add JsonNode support to TryConvert Adds conversion handling for System.Text.Json.Nodes.JsonNode values in SAM.Core.Query.TryConvert, including primitive JSON values to string, double, int, bool, and related numeric types. Object and array nodes can now be converted to JSON text when targeting string. Adds focused tests covering JsonNode primitive conversion, reuse of existing string parsing behavior, and object/array serialization.
…ject, long truncation, dead branches - TryConvertJsonNode: handle JsonValueKind.Object for IJSAMObject targets by round-tripping through the string deserialization path; fixes GetValue<SolarModel>() returning null after SetValue stores a JsonObject - long branch: use Convert.ToInt64 instead of ToInt32 (was silently truncating) - Remove duplicate else-if(int) block that used ToInt16 and was unreachable - Remove dead else-if(result is Enum) block (result is always null/default here; correct handling is already in the else-if(type_Temp.IsEnum) branch below) - Remove dead else-if(@object is int) inside the double branch (int is numeric, already caught by IsNumeric; the body used an invalid (bool) cast) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
P1: Skip NaN values in WeatherHour.ToJsonObject — System.Text.Json throws on
non-finite doubles; NaN means missing data and is not stored on read.
P1: Guard Activator.CreateInstance in JSAMObject factory with try-catch to
avoid MissingMethodException for types without parameterless ctors.
P1: Wrap JsonNode.Parse in try-catch in Convert.ToSAM JsonArray helper so
malformed/non-JSON strings return null instead of throwing.
P1: Skip zones with no related spaces in PartF Grasshopper component to
prevent Calculate(null) from falling back to processing all spaces.
P2: Restore single-object JSON support and guard Parse in IJSAMObjects<T>
string overload — now handles both {…} and [{…}] inputs safely.
P2: Use first-match semantics for duplicate names in AddResults name indexes
(spaces, panels, zones) to preserve pre-optimisation behaviour.
P2: Key SplitSystemsByZones tuples by MechanicalSystemType.Guid not Name
to prevent two distinct types sharing a display name from merging.
P2: Return Value != null from GooTransform3D.Read so a failed deserialise
reports failure at load time rather than silently carrying a null payload.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f0f0a0dff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| JsonArray valuesArray = new JsonArray(); | ||
| foreach (double value in keyValuePair.Value) | ||
| { | ||
| valuesArray.Add(value); |
There was a problem hiding this comment.
When a WeatherDay contains missing hourly measurements, the arrays commonly contain double.NaN (for example the indexer initializes a new 24-hour series with NaN for unspecified hours). This loop writes those values directly into a JsonArray; under the new System.Text.Json serialization path, ToJsonString() rejects non-finite numbers, so exporting or saving weather data with any missing hourly slots can throw instead of round-tripping. Apply the same non-finite handling used for WeatherHour before adding the value.
Useful? React with 👍 / 👎.
…nvert Extends TryConvertJsonNode to materialize List<T>/T[]/SAMCollection<T> of IJSAMObject from a JsonArray, the array sibling of the JsonObject->IJSAMObject migration fix. Adds regression tests covering list, array, SAMCollection, and the primitive-array negative case. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
double.IsFinite is unavailable on netstandard2.0 (SAM.Weather's target), breaking the Release build. Replace with the equivalent IsNaN/IsInfinity guard introduced by 5f0f0a0. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
fix: netstandard2.0-safe finite check in WeatherHour
…e-shades-linked-face-ref
…overage infrastructure feat: add SolarModel parameter and LinkedFace3D.Reference for shade-coverage infrastructure
There was a problem hiding this comment.
💡 Codex Review
https://github.com/HoareLea/SAM/blob/c06d3d0dddeaaf6effdc5fae703c2ac3babb398e/SAM.Analytical/Create/MechanicalSystem.cs#L22
External calls such as Create.MechanicalSystem(type, "AHU-1") that previously set the system Id now still compile but bind that string to namePrefix, leaving id null and changing the generated system name. Any downstream code that relies on the public factory preserving IDs (for example when creating systems before assigning them to spaces) will silently create systems without the requested Id; keep a two-argument id overload or otherwise avoid reinterpreting the existing parameter.
https://github.com/HoareLea/SAM/blob/c06d3d0dddeaaf6effdc5fae703c2ac3babb398e/SAM.Weather/Classes/GroundTemperature.cs#L234-L235
When an EPW GROUND TEMPERATURES value fails to parse, TryGetGroundTemperatures stores double.NaN in the monthly temperatures array, but this loop writes each value directly into a JsonArray. With the new System.Text.Json node path, calling ToJsonString() on a node containing NaN/Infinity throws, so weather data with a missing ground-temperature month can no longer be exported or saved; use the same Query.ToJsonNode/null handling used elsewhere for non-finite doubles.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
2 participants
Summary
This is draft PR for this quarter
Validation
How to verify or test the change.
@codex review