feat(FXC-6541): support multiple surface outputs with different frequ…#1969
feat(FXC-6541): support multiple surface outputs with different frequ…#1969NasserFlexCompute wants to merge 10 commits intomainfrom
Conversation
…encies and formats Allow the same surface to appear in multiple SurfaceOutput instances, each with its own frequency, format, and output fields. When surfaces overlap, users must provide unique `name` values. The translator emits an array of configs for multiple instances and a single object for one (backward compat). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Remove the single-instance backward-compat unwrapping — the solver handles both formats via is_object() wrapping. Update all ref JSON files and test assertions accordingly. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The name field is used in output filenames by the solver, so it must not contain path separators or null bytes. Switch from Optional[str] to Optional[FileNameString] and remove the dead empty-string entry from SURFACE_OUTPUT_DEFAULT_NAMES. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
flow360/component/simulation/validation/validation_simulation_params.py
Outdated
Show resolved
Hide resolved
…utputs - Replace SURFACE_OUTPUT_DEFAULT_NAMES constant with a has_default_name property on SurfaceOutput that reads the default from Pydantic model_fields - Always emit "name" in translated surfaceOutput JSON instead of conditionally omitting it for defaults - Sort translated surface output configs by name for stable ordering - Rename _check_duplicate_surface_usage to _check_duplicate_surface_usage_in_surface_output - Update ref JSONs and tests to match new translator output Made-with: Cursor
- Reject '%' in FileNameString to prevent snprintf UB in C++ solver - Add missing "name" key to test expected dicts (compare_values requires identical key sets) - Fix Flow360_user_variable.json ref to use the test's custom name "surface_output" instead of the default - Add pylint disable for Pydantic model_fields subscript false positive - Fix stale docstrings on translate_surface_output and check_duplicate_surface_usage Made-with: Cursor
There was a problem hiding this comment.
Pull request overview
This PR extends the simulation output model/translator to support multiple SurfaceOutput / TimeAverageSurfaceOutput instances, enabling the same surface to be emitted in multiple surface-output configs with different frequencies/formats/fields, while adding validation rules around naming and updating translator “golden” JSON references accordingly.
Changes:
- Translate
surfaceOutput/timeAverageSurfaceOutputas multiple per-instance solver configs, addingnameto each config and sorting configs by name. - Add validation requiring unique, non-default
namevalues when the same surface is used by multiple outputs of the same type; add filename-style validation for output names (now also rejects%). - Update unit/integration tests and reference solver JSONs to match the new surface output schema.
Reviewed changes
Copilot reviewed 40 out of 40 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/simulation/translator/test_output_translation.py | Updates translation tests for multi-instance surface outputs; adds new multi-output scenarios. |
| tests/simulation/translator/ref/Flow360_XV15HoverMRF.json | Updates golden solver JSON: surfaceOutput becomes an array with per-instance name. |
| tests/simulation/translator/ref/Flow360_xv15_bet_disk_unsteady_hover.json | Updates golden solver JSON for new surfaceOutput array format. |
| tests/simulation/translator/ref/Flow360_xv15_bet_disk_unsteady_hover_UDD.json | Updates golden solver JSON for new surfaceOutput array format. |
| tests/simulation/translator/ref/Flow360_xv15_bet_disk_steady_hover.json | Updates golden solver JSON for new surfaceOutput array format. |
| tests/simulation/translator/ref/Flow360_xv15_bet_disk_steady_airplane.json | Updates golden solver JSON for new surfaceOutput array format. |
| tests/simulation/translator/ref/Flow360_xv15_bet_disk_nested_rotation.json | Updates golden solver JSON for new surfaceOutput array format. |
| tests/simulation/translator/ref/Flow360_windtunnel.json | Updates golden solver JSON for new surfaceOutput array format. |
| tests/simulation/translator/ref/Flow360_user_variable.json | Updates golden solver JSON for new surfaceOutput array format (including name). |
| tests/simulation/translator/ref/Flow360_TurbFlatPlate137x97_BoxTrip.json | Updates golden solver JSON for new surfaceOutput array format. |
| tests/simulation/translator/ref/Flow360_symmetryBC.json | Updates golden solver JSON for new surfaceOutput array format. |
| tests/simulation/translator/ref/Flow360_porous_media_volume_zone.json | Updates golden solver JSON for new surfaceOutput array format. |
| tests/simulation/translator/ref/Flow360_porous_media_box.json | Updates golden solver JSON for new surfaceOutput array format. |
| tests/simulation/translator/ref/Flow360_porous_jump.json | Updates golden solver JSON for new surfaceOutput array format. |
| tests/simulation/translator/ref/Flow360_plateASI.json | Updates golden solver JSON for new surfaceOutput array format. |
| tests/simulation/translator/ref/Flow360_om6Wing.json | Updates golden solver JSON for new surfaceOutput array format. |
| tests/simulation/translator/ref/Flow360_om6wing_wall_model.json | Updates golden solver JSON for new surfaceOutput array format. |
| tests/simulation/translator/ref/Flow360_om6wing_streamlines.json | Updates golden solver JSON for new surfaceOutput array format. |
| tests/simulation/translator/ref/Flow360_om6wing_stopping_criterion_and_moving_statistic.json | Updates golden solver JSON for new surfaceOutput array format. |
| tests/simulation/translator/ref/Flow360_om6wing_SST_with_modified_C_sigma_omega1.json | Updates golden solver JSON for new surfaceOutput array format. |
| tests/simulation/translator/ref/Flow360_om6wing_SA_with_modified_C_w2.json | Updates golden solver JSON for new surfaceOutput array format. |
| tests/simulation/translator/ref/Flow360_om6Wing_SA_with_low_reynolds_correction.json | Updates golden solver JSON for new surfaceOutput array format. |
| tests/simulation/translator/ref/Flow360_om6wing_render.json | Updates golden solver JSON for new surfaceOutput array format. |
| tests/simulation/translator/ref/Flow360_om6wing_FS_with_vel.json | Updates golden solver JSON for new surfaceOutput array format. |
| tests/simulation/translator/ref/Flow360_om6wing_FS_with_vel_expression.json | Updates golden solver JSON for new surfaceOutput array format. |
| tests/simulation/translator/ref/Flow360_om6wing_FS_with_turbulence_quantities.json | Updates golden solver JSON for new surfaceOutput array format. |
| tests/simulation/translator/ref/Flow360_om6Wing_debug_type.json | Updates golden solver JSON for new surfaceOutput array format. |
| tests/simulation/translator/ref/Flow360_om6Wing_debug_point.json | Updates golden solver JSON for new surfaceOutput array format. |
| tests/simulation/translator/ref/Flow360_NestedCylindersSRF.json | Updates golden solver JSON for new surfaceOutput array format. |
| tests/simulation/translator/ref/Flow360_mirrored_surface_translation.json | Updates golden solver JSON for new surfaceOutput array format. |
| tests/simulation/translator/ref/Flow360_heatFluxCylinder.json | Updates golden solver JSON for new surfaceOutput array format. |
| tests/simulation/translator/ref/Flow360_ghost_periodic.json | Updates golden solver JSON for new surfaceOutput array format. |
| tests/simulation/translator/ref/Flow360_CHT_three_cylinders.json | Updates golden solver JSON for multi-instance surfaceOutput array format. |
| tests/simulation/service/test_integration_metadata.py | Updates integration test to handle surfaceOutput now being indexed (array). |
| tests/simulation/params/test_validators_output.py | Adds/updates validator tests for duplicate surface usage and invalid name values. |
| tests/simulation/outputs/test_filename_string.py | Adds test rejecting % in FileNameString. |
| flow360/component/simulation/validation/validation_simulation_params.py | Implements new duplicate-surface validation logic requiring unique names. |
| flow360/component/simulation/translator/solver_translator.py | Refactors surface-output translation to per-instance configs and sorts by name. |
| flow360/component/simulation/simulation_params.py | Wires the new duplicate-surface validation into SimulationParams. |
| flow360/component/simulation/outputs/outputs.py | Updates SurfaceOutput/TimeAverageSurfaceOutput naming constraints and filename validation behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…y string to solver JSON Change SurfaceOutput.name and TimeAverageSurfaceOutput.name default from a string label to None. The translator writes an empty string to the solver JSON when name is None, so the C++ solver falls back to the standard "surface" prefix. Non-None names are written as-is for disambiguation. Made-with: Cursor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 48 out of 48 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| configs = translate_surface_output(outputs, output_class, translated) | ||
| configs.sort(key=lambda c: c["name"]) | ||
| translated[output_key] = [add_unused_output_settings_for_comparison(c) for c in configs] |
There was a problem hiding this comment.
The PR description states surfaceOutput/timeAverageSurfaceOutput should be emitted as a single object when there is only one instance (backward compatibility), but this code always assigns a list. Either implement the conditional (emit dict when len(configs)==1) or update the PR description/compat notes and any downstream contract assumptions.
| def test_single_surface_output_emits_array(): | ||
| """Even a single SurfaceOutput should produce an array (solver handles both formats).""" | ||
| with SI_unit_system: | ||
| param = SimulationParams( |
There was a problem hiding this comment.
This test asserts that even a single SurfaceOutput must translate to an array, which conflicts with the PR description claiming single-instance output remains an object for backward compatibility. Align the test with the intended contract (either expect a dict for single-instance, or update the PR description/contract to “always array”).
| def translate_imported_surface_output( | ||
| output_params: list, | ||
| surface_output_class: Union[SurfaceOutput, TimeAverageSurfaceOutput], | ||
| surface_output_class: Union[Type[SurfaceOutput], Type[TimeAverageSurfaceOutput]], | ||
| coordinate_system_manager=None, | ||
| ): | ||
| """Translate imported surface output settings.""" |
There was a problem hiding this comment.
Now that multiple SurfaceOutput instances can legitimately target the same surface with different frequencies/formats, imported-surface outputs need the same treatment: translate_imported_surface_output still produces a single config by taking global settings from the first instance, which will silently ignore per-instance settings when multiple outputs reference ImportedSurface. Either extend the imported-surface translation/schema to emit per-instance configs too, or add validation to forbid multiple imported-surface outputs of the same type.
- Update translator input data files to use null for default surface output name, matching the new None default - Remove explicit name="Surface output" from services.py default params - Remove name field from service_init ref files (Pydantic excludes None) - Fix surface_output_class type annotations to use Type[] instead of instance types Made-with: Cursor
NasserFlexCompute
force-pushed
the
fxc-6541/multiple-surface-outputs
branch
from
d7c4486 to
ab52ca2
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 49 out of 49 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -595,14 +596,30 @@ def translate_surface_output( | |||
| MirroredSurface, | |||
| ), | |||
| ) | |||
| surface_output["writeSingleFile"] = get_global_setting_from_first_instance( | |||
| output_params, | |||
| surface_output_class, | |||
| "write_single_file", | |||
| ) | |||
| surface_output["writeSingleFile"] = output_instance.write_single_file | |||
| surface_output["name"] = output_instance.name if output_instance.name is not None else "" | |||
| return surface_output | |||
There was a problem hiding this comment.
_translate_single_surface_output excludes ImportedSurface from entity_type_to_include, so a SurfaceOutput that only targets imported surfaces will produce a solver config with an empty surfaces map. translate_output will still emit this empty config under surfaceOutput, in addition to importedSurfaceOutput in Step5. Consider filtering out configs where surfaces is empty (and relying on importedSurfaceOutput), or including ImportedSurface in the per-instance surface translation so the solver never sees a surfaceOutput entry with no surfaces.
| @@ -568,22 +568,23 @@ def translate_imported_surface_output( | |||
| return imported_surface_output | |||
There was a problem hiding this comment.
Support for multiple per-instance SurfaceOutput configs is added for simulation surfaces, but translate_imported_surface_output still collapses all imported-surface SurfaceOutput instances into a single config via init_output_base/global settings. This means multiple imported-surface outputs with different frequencies/formats cannot be represented. Consider emitting a per-instance list for imported surfaces as well (mirroring surfaceOutput), or adding validation to disallow multiple imported-surface SurfaceOutput instances with differing settings.
| def test_single_surface_output_emits_array(): | ||
| """Even a single SurfaceOutput should produce an array (solver handles both formats).""" | ||
| with SI_unit_system: | ||
| param = SimulationParams( | ||
| outputs=[ | ||
| SurfaceOutput( | ||
| entities=[Surface(name="wing")], | ||
| output_fields=["Cp"], | ||
| output_format="paraview", | ||
| ), | ||
| ], | ||
| ) | ||
| translated = {"boundaries": {}} | ||
| translated = translate_output(param, translated) | ||
|
|
||
| assert isinstance(translated["surfaceOutput"], list) | ||
| assert len(translated["surfaceOutput"]) == 1 | ||
| assert translated["surfaceOutput"][0]["name"] == "" |
There was a problem hiding this comment.
This test asserts that even a single SurfaceOutput translates to an array, but the PR description says the translator should emit a single object for one instance (backward compatibility). Either adjust the translator/tests to match the documented behavior (object for single, array for multiple), or update the PR description and any public docs to indicate the output is always an array.
…lator - Remove Optional from SurfaceOutput.name and TimeAverageSurfaceOutput.name since they always have a string default - Translator writes empty string to solver JSON for default names so the C++ solver falls back to the standard "surface" prefix - Fix surface_output_class type annotations to use Type[] - Restore name="Surface output" in services.py default params - Re-sort service_init ref files for CI compatibility Made-with: Cursor
NasserFlexCompute
force-pushed
the
fxc-6541/multiple-surface-outputs
branch
from
e5c4db5 to
633afeb
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 633afeb. Configure here.
| translated[output_key] = add_unused_output_settings_for_comparison(surface_output) | ||
| configs = translate_surface_output(outputs, output_class, translated) | ||
| configs.sort(key=lambda c: c["name"]) | ||
| translated[output_key] = [add_unused_output_settings_for_comparison(c) for c in configs] |
There was a problem hiding this comment.
Imported surface output not updated for per-instance translation
Medium Severity
translate_imported_surface_output still receives the full outputs list and merges all SurfaceOutput instances via init_output_base, taking frequency/format from the first instance only. Now that multiple SurfaceOutput instances with different frequencies and formats are allowed, any ImportedSurface entities across those instances will be incorrectly merged into a single config using only the first instance's settings, rather than being translated per-instance like regular surface outputs.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 633afeb. Configure here.
…encies and formats
Allow the same surface to appear in multiple SurfaceOutput instances, each with its own frequency, format, and output fields. When surfaces overlap, users must provide unique
namevalues. The translator emits an array of configs for multiple instances and a single object for one (backward compat).Note
Medium Risk
Changes the emitted solver JSON shape for
surfaceOutput/timeAverageSurfaceOutput(now arrays of per-instance configs) and adds new validation rules around output naming, which may impact downstream consumers relying on the previous single-object translation.Overview
Enables multiple
SurfaceOutput/TimeAverageSurfaceOutputinstances to target the same surface with different frequencies/formats by requiring a unique, non-defaultnamewhenever a surface is shared across outputs.Updates translation to emit per-instance solver configs:
surfaceOutputandtimeAverageSurfaceOutputbecome lists (sorted byname) with per-outputwriteSingleFileand an explicitnamefield (""when default), and adjusts validators/tests and reference JSON accordingly. Also tightens output-name validation by rejecting%(in addition to/and null bytes) to avoid solver formatting conflicts.Reviewed by Cursor Bugbot for commit 633afeb. Bugbot is set up for automated code reviews on this repo. Configure here.