-
Notifications
You must be signed in to change notification settings - Fork 9
[FXC-3564][FXC-979] ForceOutput and its stopping criterion #1519
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
c87d8f8 to
a611fc9
Compare
0c3f55f to
48ac55a
Compare
bf011b3 to
0a9b5ea
Compare
5dc97e5 to
a78def5
Compare
1c5d40e to
b8854e8
Compare
b8854e8 to
6aec51c
Compare
3720942 to
f7b6ddb
Compare
|
@codex review the PR |
|
To use Codex here, create a Codex account and connect to github. |
|
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Flow360/flow360/component/simulation/translator/solver_translator.py
Lines 970 to 974 in f7b6ddb
| if has_instance_in_list(outputs, SurfaceIntegralOutput): | |
| process_user_variables_for_integral(outputs) | |
| integral_output = translate_monitor_output( | |
| outputs, SurfaceIntegralOutput, inject_surface_list_info | |
| ) |
The translator still builds monitorOutput only from ProbeOutput and SurfaceIntegralOutput; there is no branch anywhere in get_solver_json that serializes the newly added ForceOutput monitor type (now part of MonitorOutputType). Any ForceOutput configured in SimulationParams will be silently omitted from the solver JSON, so total force coefficients and any stopping criteria based on them will never be produced.
ℹ️ 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".
| or validation_info.physics_model_dict is None | ||
| or validation_info.physics_model_dict.get(model) is None | ||
| ): | ||
| raise ValueError("The model does not exist in simulation params' models list.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be similarly done as the materialization function for the text expressions. Ad-hoc deserialization will make these 2 models independent which causes issue. Will this impact anything?
d07bebe to
3a88385
Compare
…re expanded before validation
…ng criterion check
…andard_deviation" and "range"
…the global validation context.
…iterion when it does not exist in outputs list
…he related translator and validation unit test
bfad398 to
e6bf758
Compare
| # Check if required context attributes are available | ||
| if required_context: | ||
| for attr_name in required_context: | ||
| if not hasattr(param_info, attr_name) or getattr(param_info, attr_name) is None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first criteria should raise. It indicates a bug.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added ValueError and unit test.
No description provided.