-
Notifications
You must be signed in to change notification settings - Fork 57
fix: example deprecation warnings and improvements #4579
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
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.
Pull Request Overview
This PR fixes deprecation warnings by updating all references from direct solver session settings access (solver.setup.models) to the correct settings-based access pattern (solver.settings.setup.models). Additionally, it adds PEP 723 style dependencies to example scripts and includes minor improvements throughout.
- Updates deprecated direct access patterns to settings-based access across all test files and examples
- Adds PEP 723 dependency declarations to example scripts for standalone execution
- Includes miscellaneous code improvements and cleanups
Reviewed Changes
Copilot reviewed 67 out of 68 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| tests/ | Updates test files to use solver.settings instead of direct access patterns |
| examples/ | Adds PEP 723 dependency headers and updates deprecated settings access |
| src/ | Updates core library files to use settings-based access pattern |
| doc/ | Updates documentation examples to reflect new access patterns |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Anyone have any opinions on #4512 (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.
Pull request overview
Copilot reviewed 67 out of 68 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for n_ind, variable in enumerate(varname): | ||
| if len(variable) >= 4 and variable[:4] == "mean": | ||
| session.solution.report_definitions.surface["mag-report"] = { | ||
| if variable.startswith("mean"): |
Copilot
AI
Dec 2, 2025
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 change from if len(variable) >= 4 and variable[:4] == "mean" to if variable.startswith("mean") is an improvement, but it changes behavior slightly. The original checked for exactly 4 characters before comparing, while startswith() would match any variable starting with "mean" (e.g., "means", "meaningful"). Consider if this behavior change is intentional.
| if variable.startswith("mean"): | |
| if len(variable) >= 4 and variable[:4] == "mean": |
| ####################################################################################### | ||
| # Import required libraries/modules | ||
| # ===================================================================================== | ||
| import csv |
Copilot
AI
Dec 2, 2025
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.
[nitpick] The csv import is added but the code still has a mix of approaches. Consider whether all file I/O should use the csv module consistently throughout the file for better maintainability.
| # --------------------------------------------------------------- | ||
|
|
||
| session = pyfluent.launch_fluent(precision="double", processor_count=2, version="3d") | ||
| session = pyfluent.launch_fluent(precision="double", processor_count=2, dimension=3) |
Copilot
AI
Dec 2, 2025
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 parameter name has been changed from version="3d" to dimension=3. While this is likely a correction, verify that dimension=3 is the correct parameter name for the API, as it significantly changes the meaning (from a version identifier to a dimensional specification).
| session = pyfluent.launch_fluent(precision="double", processor_count=2, dimension=3) | |
| session = pyfluent.launch_fluent(precision="double", processor_count=2, solver_mode="3d") |
| solver_session = pyfluent.launch_fluent( | ||
| dimension=3, | ||
| precision="double", | ||
| processor_count=4, |
Copilot
AI
Dec 2, 2025
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 dimension=3 parameter was removed from the launch_fluent call. If 3D is the default, this is fine, but if the example specifically requires 3D simulation, removing this parameter could lead to incorrect behavior.
| processor_count=4, | |
| processor_count=4, | |
| dimension=3, |
| precision="double", | ||
| processor_count=2, | ||
| version="3d", | ||
| processor_count=4, |
Copilot
AI
Dec 2, 2025
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.
[nitpick] The processor count was changed from 2 to 4. While this may improve performance, it could cause issues on systems with fewer than 4 processors. Consider documenting this requirement or making it configurable.
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.
Pull request overview
Copilot reviewed 67 out of 68 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| s.split("\n")[-2].split("(")[0] | ||
| == r"<solver_session>.settings.file.read_case" | ||
| ) | ||
| assert s.split("\n")[-2].split("(")[0] == r"<solver_session>.file.read_case" |
Copilot
AI
Dec 2, 2025
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 assertion on line 69 duplicates the assertion from the conditional on line 68 for versions >= 25.1. Both branches now assert the same expected value (r\"<solver_session>.file.read_case\"), making the conditional check redundant. Consider removing the version check and using a single assertion, or verify if different behavior is expected for different versions.
|
|
||
| create_output_param = solver_session.tui.define.parameters.output_parameters.create | ||
| create_output_param = ( | ||
| solver_session.tui.define.settings.parameters.output_parameters.create |
Copilot
AI
Dec 2, 2025
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 TUI path has been changed from tui.define.parameters.output_parameters.create to tui.define.settings.parameters.output_parameters.create, adding an extra settings level. Verify this is the correct TUI command path, as incorrect TUI paths will cause runtime errors.
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.
Pull request overview
Copilot reviewed 67 out of 68 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ####################################################################################### | ||
| # Import required libraries/modules | ||
| # ===================================================================================== | ||
| import csv |
Copilot
AI
Dec 2, 2025
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.
[nitpick] The import statement for 'csv' has been added but 'open' is being replaced with 'Path.cwd().open()'. Consider moving the csv import closer to where it's used (line 247) for better code organization, or ensure the import order follows PEP 8 guidelines (standard library imports together).
| #################################################################################### | ||
| # Launch Fluent session with solver mode and print Fluent version | ||
| # ================================================================================== | ||
|
|
Copilot
AI
Dec 2, 2025
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.
[nitpick] The dimension=3 parameter was removed from the launch_fluent call. If this was intentional because 3D is the default, consider adding a comment explaining this for clarity, as the removal might confuse future readers.
| # Note: The 'dimension=3' parameter is omitted because 3D is the default for launch_fluent. |
| "FileName": wing_intermediary_file, | ||
| } | ||
| ) | ||
|
|
Copilot
AI
Dec 2, 2025
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 line meshing_session.upload(wing_intermediary_file) was removed before geo_import.Execute(). Ensure that the file upload is no longer needed or is handled elsewhere in the workflow, as this could cause the import to fail if the file is not available.
| meshing_session.upload(wing_intermediary_file) |
| # parts. | ||
|
|
||
| meshing_session.upload(import_file_name) | ||
| meshing_session.PartManagement.InputFileChanged( |
Copilot
AI
Dec 2, 2025
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.
Similar to the previous comment, meshing_session.upload(import_file_name) was removed. Verify that the file upload is handled elsewhere or is no longer required for the workflow to function correctly.
| ################################################################# | ||
| # Launch Fluent session with solver mode and print Fluent version | ||
| # =============================================================== | ||
|
|
Copilot
AI
Dec 2, 2025
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.
[nitpick] The processor count was changed from 2 to 4, and version=\"3d\" was removed. Document or verify that this change in processor count is intentional and appropriate for the example's resource requirements.
| # The processor count is set to 4 to provide sufficient computational resources | |
| # for the DOE and ML example, which can benefit from parallel processing. | |
| # The default solver version is "3d", so the explicit version argument was removed. | |
| # Adjust processor_count as needed for your hardware. |
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.
Pull request overview
Copilot reviewed 67 out of 68 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| graphics.views.camera.position = [1.70, 1.14, 0.29] | ||
| graphics.views.camera.up_vector = [-0.66, 0.72, -0.20] |
Copilot
AI
Dec 2, 2025
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 camera position and up_vector are now set using direct assignment instead of method calls (.position() and .up_vector()). Verify that the settings API supports direct assignment for these properties and that the behavior is equivalent.
| graphics.views.camera.position = [1.70, 1.14, 0.29] | |
| graphics.views.camera.up_vector = [-0.66, 0.72, -0.20] | |
| graphics.views.camera.position([1.70, 1.14, 0.29]) | |
| graphics.views.camera.up_vector([-0.66, 0.72, -0.20]) |
| mode="meshing", | ||
| dimension=3, | ||
| precision="double", | ||
| processor_count=4, | ||
| ) |
Copilot
AI
Dec 2, 2025
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 dimension=3 parameter was removed from launch_fluent. Ensure the default dimension setting is appropriate for this conjugate heat transfer case.
| solver_session.file.read_case(file_name=import_file_name) | ||
| solver_session.solution.run_calculation.iter_count = 100 | ||
| solver_session.settings.file.read_case(file_name=import_file_name) | ||
| solver_session.settings.solution.run_calculation.parameters.iter_count = 100 |
Copilot
AI
Dec 2, 2025
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 iter_count assignment was changed from solver_session.settings.solution.run_calculation.iter_count to solver_session.settings.solution.run_calculation.parameters.iter_count. Verify this is the correct path for setting iteration count in the parametric workflow context.
| solver_session.settings.solution.run_calculation.parameters.iter_count = 100 | |
| solver_session.settings.solution.run_calculation.iter_count = 100 |
Context
When solver session settings access was deprecated directly on solver the code wasn't updated I've also sprinkled in a couple of small improvements to the examples
Change Summary
Updated all references direct access patterns (
solver.setup.models) to the correct settings-based access pattern (solver.settings.setup.models) along with adding PEP 723 styles dependencies to the examples to make them easier to run as standalone scripts.Rationale
Fixes all the deprecation warnings
Impact
Changes basically everywhere