Add optical table options to Trough model#2182
Add optical table options to Trough model#2182taylorbrown75 wants to merge 4 commits intodevelopfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds UI and default-data support for selecting an “optical model type” for Physical Trough collectors, enabling use of optical tables (solar position or field incidence) in addition to IAM coefficients.
Changes:
- Adds a RadioChoice selector and OpticalTable DataMatrix inputs to collector type UI pages (Types 1–4)
- Aggregates per-collector optical model selections into a single
opt_modelarray in the collector header - Updates multiple default JSON configurations to include
DISP_opt_model_*andOpticalTable_*initial values
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| deploy/runtime/ui/Physical Trough Collector Type 1.json | Adds optical model selector + optical table matrix and related callbacks for collector type 1 |
| deploy/runtime/ui/Physical Trough Collector Type 2.json | Adds optical model selector + optical table matrix and related callbacks for collector type 2 |
| deploy/runtime/ui/Physical Trough Collector Type 3.json | Adds optical model selector + optical table matrix and related callbacks for collector type 3 |
| deploy/runtime/ui/Physical Trough Collector Type 4.json | Adds optical model selector + optical table matrix and related callbacks for collector type 4 |
| deploy/runtime/ui/Physical Trough Collector Header.json | Adds opt_model aggregated variable and equations to feed compute layer |
| deploy/runtime/startup.lk | Makes collector type 1 page collapsible like types 2–4 |
| deploy/runtime/defaults/Physical Trough_None.json | Seeds DISP_opt_model_*, OpticalTable_*, and related defaults |
| deploy/runtime/defaults/Physical Trough_LCOE Calculator.json | Seeds DISP_opt_model_*, OpticalTable_*, and related defaults |
| deploy/runtime/defaults/Physical Trough_Commercial.json | Seeds DISP_opt_model_*, OpticalTable_*, and related defaults |
| deploy/runtime/defaults/Physical Trough IPH_Single Owner.json | Seeds DISP_opt_model_* and placeholder OpticalTable_* defaults |
| deploy/runtime/defaults/Physical Trough IPH_None.json | Seeds DISP_opt_model_* and optical tables (mixed full + placeholder) |
| deploy/runtime/defaults/Physical Trough IPH_LCOH Calculator.json | Seeds DISP_opt_model_* and optical tables (mixed full + placeholder) |
| deploy/runtime/defaults/Physical Trough IPH_Commercial.json | Seeds DISP_opt_model_* and optical tables (mixed full + placeholder) |
Comments suppressed due to low confidence (2)
deploy/runtime/ui/Physical Trough Collector Header.json:1
acollectoris used without being initialized within thisdefine()block. Unless thedefine()scope implicitly provides a pre-sized array, this can throw at runtime or (worse) reuse a stale array from another equation depending on the interpreter. Initialize a local array inside this function (and ensure it has length 4) before assigning indices, then return it.
{
deploy/runtime/ui/Physical Trough Collector Type 1.json:1
apply_collector_library_1now forcibly setsDISP_opt_model_1to IAM coefficients (2) every time the library is applied. This overrides any user-selected optical model type and may be unexpected (especially if a library entry is intended to be used with an optical table). Consider preserving the current selection, or only setting this default when the model type is unset/invalid (or if the library explicitly indicates it uses IAM coefficients).
{
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| } | ||
| }, | ||
| "GroupBox": { |
There was a problem hiding this comment.
This JSON object defines the FormObjects key \"GroupBox\" more than once. In JSON, duplicate keys are invalid/undefined behavior (many parsers will keep only the last one), which can break the UI layout by overwriting the earlier GroupBox definition. Rename the second GroupBox entry key to a unique identifier (e.g., \"GroupBox_optical_angles\") so all form objects are preserved.
| "GroupBox": { | |
| "GroupBox_collector_geometry": { |
| } | ||
| } | ||
| }, | ||
| "GroupBox": { |
There was a problem hiding this comment.
This JSON object defines the FormObjects key \"GroupBox\" more than once. In JSON, duplicate keys are invalid/undefined behavior (many parsers will keep only the last one), which can break the UI layout by overwriting the earlier GroupBox definition. Rename the second GroupBox entry key to a unique identifier (e.g., \"GroupBox_optical_angles\") so all form objects are preserved.
| "GroupBox": { | |
| "GroupBox_optical_angles": { |
| } | ||
| } | ||
| }, | ||
| "GroupBox": { |
There was a problem hiding this comment.
Duplicate \"GroupBox\" keys under FormObjects are invalid JSON / can cause one GroupBox to overwrite the other at parse-time. Give the newly added GroupBox a unique key name to avoid losing the original GroupBox definition.
| "GroupBox": { | |
| "GroupBox1": { |
| } | ||
| } | ||
| }, | ||
| "GroupBox": { |
There was a problem hiding this comment.
Duplicate \"GroupBox\" keys under FormObjects are invalid JSON / can cause one GroupBox to overwrite the other at parse-time. Give the newly added GroupBox a unique key name to avoid losing the original GroupBox definition.
| "GroupBox": { | |
| "GroupBox2": { |
| } | ||
| } | ||
| }, | ||
| "GroupBox": { |
There was a problem hiding this comment.
There are two \"GroupBox\" entries in FormObjects. JSON duplicate keys are not safe and can result in only one GroupBox being present at runtime. Rename the new GroupBox key to a unique object id.
| "GroupBox": { | |
| "GroupBox1": { |
| "ObjectProperties": { | ||
| "Name": { | ||
| "Type": 5.0, | ||
| "String": "object 4" |
There was a problem hiding this comment.
The GroupBox Name value (\"object 4\") is generic and does not describe its purpose (caption suggests it contains solar position/incidence angle controls). Use a descriptive and stable identifier (e.g., \"optical_angles_group_1\") to make future UI scripting/debugging clearer.
| "String": "object 4" | |
| "String": "optical_angles_group_1" |
| }, | ||
| "Items": { | ||
| "Type": 6.0, | ||
| "StringList": "Solar position table|Field incidence table|Incidence angle modifier coefficients" |
There was a problem hiding this comment.
The RadioChoice display items and the variable IndexLabels do not match for the first entry (\"Solar position table\" vs \"Solar position\"). If the UI framework relies on exact index-label correspondence (e.g., for serialization/import/export), this mismatch can cause confusing labels or mapping issues. Make the strings consistent across Items.StringList and VarDatabase.IndexLabels.
| "StringList": "Solar position table|Field incidence table|Incidence angle modifier coefficients" | |
| "StringList": "Solar position|Field incidence table|Incidence angle modifier coefficients" |
| "Label": "Optical model type", | ||
| "Units": "", | ||
| "Group": "Physical Trough Collector Type 1", | ||
| "IndexLabels": "Solar position|Field incidence table|Incidence angle modifier coefficients", |
There was a problem hiding this comment.
The RadioChoice display items and the variable IndexLabels do not match for the first entry (\"Solar position table\" vs \"Solar position\"). If the UI framework relies on exact index-label correspondence (e.g., for serialization/import/export), this mismatch can cause confusing labels or mapping issues. Make the strings consistent across Items.StringList and VarDatabase.IndexLabels.
| "IndexLabels": "Solar position|Field incidence table|Incidence angle modifier coefficients", | |
| "IndexLabels": "Solar position table|Field incidence table|Incidence angle modifier coefficients", |
| 2.0, | ||
| 1.0, | ||
| 2.0, | ||
| 1.0 | ||
| ] | ||
| ], | ||
| "OpticalTable_2": [ | ||
| [ | ||
| 2.0, | ||
| 1.0, | ||
| 2.0, | ||
| 1.0 | ||
| ] | ||
| ], | ||
| "OpticalTable_3": [ | ||
| [ | ||
| 2.0, | ||
| 1.0, | ||
| 2.0, | ||
| 1.0 | ||
| ] | ||
| ], | ||
| "OpticalTable_4": [ | ||
| [ | ||
| 2.0, | ||
| 1.0, | ||
| 2.0, |
There was a problem hiding this comment.
The default OpticalTable_1 (and similarly _2/_3/_4 in this file) is a 1x4 placeholder matrix, unlike other defaults that seed a structured table with header row/column values. If a user selects a table-based optical model, this placeholder shape is likely not a valid optical table and could cause downstream parsing/compute failures. Provide a valid minimal table format consistent with what the compute module expects (or seed the same full template table used in the other defaults).
| 2.0, | |
| 1.0, | |
| 2.0, | |
| 1.0 | |
| ] | |
| ], | |
| "OpticalTable_2": [ | |
| [ | |
| 2.0, | |
| 1.0, | |
| 2.0, | |
| 1.0 | |
| ] | |
| ], | |
| "OpticalTable_3": [ | |
| [ | |
| 2.0, | |
| 1.0, | |
| 2.0, | |
| 1.0 | |
| ] | |
| ], | |
| "OpticalTable_4": [ | |
| [ | |
| 2.0, | |
| 1.0, | |
| 2.0, | |
| 0.0, | |
| 0.0 | |
| ], | |
| [ | |
| 0.0, | |
| 1.0 | |
| ] | |
| ], | |
| "OpticalTable_2": [ | |
| [ | |
| 0.0, | |
| 0.0 | |
| ], | |
| [ | |
| 0.0, | |
| 1.0 | |
| ] | |
| ], | |
| "OpticalTable_3": [ | |
| [ | |
| 0.0, | |
| 0.0 | |
| ], | |
| [ | |
| 0.0, | |
| 1.0 | |
| ] | |
| ], | |
| "OpticalTable_4": [ | |
| [ | |
| 0.0, | |
| 0.0 | |
| ], | |
| [ | |
| 0.0, |
| 2.0, | ||
| 1.0, | ||
| 2.0, | ||
| 1.0 |
There was a problem hiding this comment.
OpticalTable_4 is seeded as a 1x4 placeholder matrix here, while OpticalTable_1/2/3 in the same file are seeded as full table templates. This inconsistent default shape is likely to break table-based optical model selection for collector type 4. Seed OpticalTable_4 with the same valid table format used for the other collector types.
| 2.0, | |
| 1.0, | |
| 2.0, | |
| 1.0 | |
| 0.0, | |
| 0.0, | |
| 5.0, | |
| 10.0, | |
| 15.0, | |
| 20.0, | |
| 25.0, | |
| 30.0, | |
| 35.0, | |
| 40.0, | |
| 45.0, | |
| 50.0, | |
| 55.0, | |
| 60.0, | |
| 65.0, | |
| 70.0, | |
| 75.0, | |
| 80.0, | |
| 85.0, | |
| 90.0 | |
| ], | |
| [ | |
| 0.0, | |
| 1.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0 | |
| ], | |
| [ | |
| 10.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0 | |
| ], | |
| [ | |
| 20.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0 | |
| ], | |
| [ | |
| 30.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0 | |
| ], | |
| [ | |
| 40.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0 | |
| ], | |
| [ | |
| 50.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0 | |
| ], | |
| [ | |
| 60.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0 | |
| ], | |
| [ | |
| 70.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0 | |
| ], | |
| [ | |
| 80.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0 | |
| ], | |
| [ | |
| 90.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0, | |
| 0.0 |
Pull Request Template
Description
Add ability to use Optical Table to define solar position or IAM derates, rather than only IAM coefficients.
Follows fresnel functionality.
Corresponding branches and PRs:
wex, lk: develop
ssc: trough_optical_table
Unit Test Impact:
No expected change in unit test results. Defaults to IAM coefficients.
Checklist