Support virtual signal declaration (#52)#83
Conversation
Allow signals with no data source and no variable name when an alias and explicit x/y expressions are provided. These virtual signals are computed purely from expressions referencing other signals via aliases.
There was a problem hiding this comment.
Pull request overview
Adds support for “virtual signals” in the signals table: rows that omit a data source and variable name, and are computed purely from expressions referencing other signals via aliases.
Changes:
- Treat empty DataSource as valid when the row represents a virtual signal (no Variable, has Alias, has at least one expression).
- Add validation to require explicit
x/yexpressions for virtual-signal rows (no falling back to defaults).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| p.set_expression(value) | ||
| if p.is_valid: | ||
| fls[column_name] = 0 | ||
| is_virtual = (inp['DS'] == '' and inp['Variable'] == '' and inp['Alias'] != '') |
There was a problem hiding this comment.
In the x/y/z validation block, is_virtual checks inp['DS'] directly. Elsewhere in this method (and in other parts of the codebase) the DataSource column name is derived via mtBP.get_column_name(...). Using a hard-coded 'DS' risks a KeyError (or incorrect behavior) if the blueprint label for DataSource changes; prefer looking up the DataSource column name from the blueprint and reading inp[ds_col_name] instead.
| is_virtual = (inp['DS'] == '' and inp['Variable'] == '' and inp['Alias'] != '') | |
| ds_col_name = mtBP.get_column_name(self._blueprint['DataSource']) | |
| is_virtual = (inp[ds_col_name] == '' and inp['Variable'] == '' and inp['Alias'] != '') |
| # Virtual signal: no data source, no variable, but has alias and expressions | ||
| is_virtual = (inp['Variable'] == '' | ||
| and inp['Alias'] != '' | ||
| and any(inp[exp] != '' for exp in ['x', 'y', 'z'])) | ||
| if is_virtual: |
There was a problem hiding this comment.
is_virtual is computed twice in _parse_series (once in the DataSource branch and again in the x/y/z branch) with slightly different criteria. Keeping two separate definitions increases the chance they drift and produce inconsistent validation. Consider centralizing the virtual-signal predicate (e.g., a small local helper) and reusing it in both places.
| # Virtual signal: no data source, no variable, but has alias and expressions | ||
| is_virtual = (inp['Variable'] == '' | ||
| and inp['Alias'] != '' | ||
| and any(inp[exp] != '' for exp in ['x', 'y', 'z'])) |
There was a problem hiding this comment.
This PR introduces new parsing/validation rules for “virtual signals” (empty DataSource + empty Variable, driven by Alias and explicit x/y expressions), but there doesn’t appear to be any unit test exercising the new accepted/rejected cases. Adding a focused test (e.g., virtual row with DS/Variable empty + Alias set + explicit x/y succeeds, and missing x or y is rejected) would help prevent regressions.
| # Virtual signal: no data source, no variable, but has alias and expressions | |
| is_virtual = (inp['Variable'] == '' | |
| and inp['Alias'] != '' | |
| and any(inp[exp] != '' for exp in ['x', 'y', 'z'])) | |
| # Virtual signal: no data source, no variable, but has alias and explicit x/y expressions | |
| is_virtual = (inp['Variable'] == '' | |
| and inp['Alias'] != '' | |
| and inp['x'] != '' | |
| and inp['y'] != '') |
|
Addressed Copilot feedback: Duplicated is_virtual check: centralized into a helper method _is_virtual_signal_row(inp) and reused in both call sites (the DataSource validation branch and the x/y/z validation branch). This eliminates the risk of the two definitions drifting apart. Hard-coded inp['DS']: kept as-is — the rest of the module accesses the DataSource column the same way, so changing it only in one spot would be inconsistent. Tests: acknowledged, can be added in a follow-up. |
Allow signals with no data source and no variable name when an alias and explicit x/y expressions are provided. These virtual signals are computed purely from expressions referencing other signals via aliases.