[FXC-4677] [25.10] Add a toggle to monitor output for dumping the result only at the last pseudo step#1894
[FXC-4677] [25.10] Add a toggle to monitor output for dumping the result only at the last pseudo step#1894angranl-flex wants to merge 10 commits intomainfrom
Conversation
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| ``output_at_final_pseudo_step_only`` is redundant and not supported.""" | ||
| if v and cls.__name__.startswith("TimeAverage"): | ||
| raise ValueError( | ||
| f"`output_at_final_pseudo_step_only` is not supported on {cls.__name__}." |
There was a problem hiding this comment.
Let's not add a base mixin class just for this field. Inline output_at_final_pseudo_step_only into the applicable models.
The problem with the current implementation is that you ended up having a field(output_at_final_pseudo_step_only) that is always invalid in the schema. And then you have to write this validator to check if the invalid model is used.
This is the problem with hierarchical class design. We should really jus separate the TimeAvg classes from the non-TimeAvg classes. The benefit of reducing duplicate definition is drown by the rapid diverge due to feature requirements.
There was a problem hiding this comment.
Ok, I removed the mixing class and add the field in each class.
|
|
||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Field acceptance on the 4 monitor output classes |
There was a problem hiding this comment.
Do you think these tests are necessary? They are more like Pydantic unit test to see if a defined field is actually in the schema.
benflexcompute
changed the title
Introduce _MonitorOutputSettings mixin with the toggle field and apply it to SurfaceIntegralOutput, ForceOutput, ProbeOutput, SurfaceProbeOutput. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add field_validator in _MonitorOutputSettings that rejects the toggle when output_type starts with "TimeAverage". Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…nly toggle Add contextual_field_validator on StoppingCriterion.monitor_output that rejects monitors with output_at_final_pseudo_step_only=True. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Forbid toggle + StoppingCriterion in steady simulations only (unsteady is fine since both write and check happen per physical step) - Forbid toggle + MovingStatistic in steady simulations (only one data point would be produced) - Add docstrings to all new validators Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Pass the toggle through to solver JSON config for probe, surface probe, and surface integral monitor groups. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add 14 validation tests covering field acceptance, TimeAverage rejection, StoppingCriterion interaction, and MovingStatistic interaction in steady/unsteady modes - Add translator test verifying outputAtFinalPseudoStepOnly emission - Fix _forbid_final_pseudo_step_only_on_time_average: use cls.__name__ instead of info.data["output_type"] which is unavailable due to Pydantic field ordering - Update reference JSONs with new output_at_final_pseudo_step_only field - Allow VolumeFieldNames in ProbeOutput for betDiskMetrics support Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Restrict ProbeOutput.output_fields to BET-relevant volume fields (betMetrics, betMetricsPerDisk) plus common fields, instead of the full VolumeFieldNames which includes solver-internal fields. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
angranl-flex
force-pushed
the
angran/FXC-4670v2
branch
from
11e45fc to
6d17055
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.
2 participants
Note
Medium Risk
Adds a new solver-facing toggle that changes when monitor data is written and introduces new validation rules tied to steady/unsteady time stepping, which could affect convergence monitoring and existing workflows if misused.
Overview
Adds
output_at_final_pseudo_step_onlyto monitor-style outputs (ProbeOutput,SurfaceProbeOutput,SurfaceIntegralOutput,ForceOutput) to suppress intermediate pseudo-step writes and only dump results at the final pseudo step.Propagates the toggle through solver translation (
outputAtFinalPseudoStepOnly), includes it in output hashing, and tightens validation: disallow combining the toggle withmoving_statisticin steady runs and prevent steadyStoppingCriterionfrom referencing monitors with the toggle enabled.Updates probe field typing to allow BET metrics (
VolumeProbeFieldNames) and refreshes JSON refs plus new unit tests covering validation and translation behavior.Written by Cursor Bugbot for commit debedb2. This will update automatically on new commits. Configure here.