Conversation
…functional due to various settings API and other github side pull requests not being merged or fixed - We use the much more descriptive object oriented API with ORM style `create()` and `get()` methods that are more clear in intent - I've tried to get rid of TUI based examples and where not possible I've at least described what the parameters are - I've colocated the units next to their values - Removed the dictionary state assignment based APIs in favour of direct attribute access as it will leave users in a more recoverable state and is easier to type (both type hinting and on the keyboard) - Used tuples for vectors rather than lists to allow for better type hinting in the future checking 2D vs 3D cases - Used pathlib instead of os.path - Added calls to solver.upload(filename) after the example is downloaded if the mode is Meshing for based workflows - Changed all the pyfluent.lanuch_fluent to it's appropriate pyfluent.ModeClass.from_install - Stored the results of .create() and use that to apply next variable assignments if appropriate rather than performing another lookup - I've tried to reduce the stringly-typedness of the API by reusing objects (where things are explicitly suffixed with name I've tried to use the `name()` method though. In practice though i don't like this and we should be instead passing round the full objects e.g. `custom_field_function_name=omega_r.name()` where `omega_r` is a `custom_field_function`. I think it makes more sense to pass a str | custom_field_function to a parameter called `custom_field_function` than passing the same type to `custom_field_function_name` which would work if you use another one of my PRs) - I've tried to update fields to use VariableCatalog though many are missing due to the fields not existing in VariableCatalog or VariableCatalog.fluent - Improved general pythonicness of code/algorithms/loops etc NB I've updated some of the examples @MohammedAnsys has PRs open for that looked like they were ready for merging NBB I 've not switched the save locations for all these to examples_path, though that might be a good idea in the future
github-actions
bot
added
documentation
Gobot1234
changed the title
Gobot1234
left a comment
Gobot1234
left a comment
There was a problem hiding this comment.
Will finish review later
| busbar_material = SolidMaterial.create( | ||
| solver, name="busbar_material", chemical_formula="bus" | ||
| ) | ||
| busbar_material.uds_diffusivity.option = "value" | ||
| busbar_material.uds_diffusivity.value = 3.541e7 * S / m # Copper-like conductivity |
There was a problem hiding this comment.
| busbar_material = SolidMaterial.create( | |
| solver, name="busbar_material", chemical_formula="bus" | |
| ) | |
| busbar_material.uds_diffusivity.option = "value" | |
| busbar_material.uds_diffusivity.value = 3.541e7 * S / m # Copper-like conductivity | |
| busbar_material = SolidMaterial.create( | |
| solver, | |
| name="busbar_material", | |
| chemical_formula="bus", | |
| uds_diffusivity=3.541e7 * S / m, # Copper-like conductivity | |
| ) |
honestly would be surprised this didn't work
| # '*bar*' matches any zone containing 'bar', | ||
| # '|*tabzone*' matches any zone containing 'tabzone' → OR logic |
There was a problem hiding this comment.
Deleted because it should be explained further up what this does
| regolith_reports = [ | ||
| report_defs.surface.create( | ||
| name=f"regolith-layer-{i}-temp", | ||
| report_type="surface-facetavg", |
There was a problem hiding this comment.
NB check if there's enums for these cause I don't think there are, do we need to auto gen them? (#4891)
| from ansys.fluent.core.generated.solver.settings_builtin import Fluxes, Vector | ||
| from ansys.fluent.core.generated.solver.settings_builtin_261 import Materials, read_case | ||
| from ansys.fluent.core.solver import ( |
There was a problem hiding this comment.
import merge needed
| from ansys.fluent.core.generated.solver.settings_builtin import ( | ||
| BoundaryCondition, | ||
| Mesh, | ||
| PlaneSurface, | ||
| ReportFile, | ||
| ReportPlot, | ||
| Residual, | ||
| ) | ||
| from ansys.fluent.core.generated.solver.settings_builtin_261 import write_case, write_case_data | ||
| from ansys.fluent.core.solver import ( |
There was a problem hiding this comment.
merge imports again
| ] | ||
| general = General(solver) | ||
| general.operating_conditions.gravity.enable = True | ||
| general.operating_conditions.gravity.components = [0.0, 0.0, -g] |
| # Residuals criteria | ||
| residuals_options = solver_session.settings.solution.monitor.residual | ||
| residuals_options.equations["continuity"].absolute_criteria = 0.0001 | ||
| residuals_options.equations["continuity"].monitor = True # Enable continuity residuals |
| # ===================================================================================== | ||
| session = pyfluent.launch_fluent(mode="meshing", cleanup_on_exit=True) | ||
| print(session.get_fluent_version()) | ||
| meshing = pyfluent.Meshing.from_install(cleanup_on_exit=True, fluent_path=r"C:\ANSYSDev\v261\fluent\ntbin\win64\fluent.exe") |
There was a problem hiding this comment.
Other team members can comment here before you change anything due to the following observation.
One reason to avoid meshing = / solver = is that meshing and solver are protected global names in Fluent's Python console. Code that assigns to them would attempt (and fail) to overwrite those globals if the script were reused there, making the script unusable in that environment.
There was a problem hiding this comment.
Ah right, I was thinking it'd be clearer for using pyconsole because you can just skip that step but then things work for you without having to change variable names, maybe it should be clearer though that this is optional in pyconsole using a separate section
There was a problem hiding this comment.
...unfortunate because I much prefer these names.
| save_path=os.getcwd(), | ||
| save_path=Path.cwd(), | ||
| ) | ||
| meshing.upload(geometry_filename) |
There was a problem hiding this comment.
Seeing upload() calls like this one is surprising given that the upload should be taken care of seamlessly due to the geometry import operation. Either the script's explicit upload is redundant or you're aware of a limitation.
There was a problem hiding this comment.
This was for PIM based environments so customers only need to change the launch method refs TFS/1365452
| session.settings.setup.models.viscous.model = "k-epsilon" | ||
| session.settings.setup.models.viscous.k_epsilon_model = "realizable" | ||
| session.settings.setup.models.viscous.options.curvature_correction = True | ||
| air = FluidMaterial.get(solver, name="air") |
There was a problem hiding this comment.
As you'll already know, this change depends on another PR you have open. In that PR, we looked into adding this (preferred) style: air = FluidMaterial(solver).get(name="air"). Is that something we're still looking into?
There was a problem hiding this comment.
I didn't realise that style was preferred by you for all situations I thought that was for cases where Fluid Material was repeated instantiated
| workflow.TaskObject["Describe Geometry"].Arguments = { | ||
| "CappingRequired": "Yes", | ||
| "SetupType": "The geometry consists of only fluid regions with no voids", | ||
| } |
There was a problem hiding this comment.
I'm not on testing the new workflow so probably don't have the time to fix these 😔
There was a problem hiding this comment.
@Gobot1234 In the new version, the values are simply true and "fluid".
| "k", | ||
| "epsilon", | ||
| ): | ||
| residual.equations[monitor].absolute_criteria = 1e-4 |
| methods.p_v_coupling.flow_scheme = "Coupled" | ||
|
|
||
| discretization_scheme = methods.spatial_discretization.discretization_scheme | ||
| discretization_scheme["pressure"] = "second-order" | ||
| discretization_scheme["k"] = "second-order-upwind" | ||
| discretization_scheme["epsilon"] = "second-order-upwind" |
There was a problem hiding this comment.
@Gobot1234 For some of these, perhaps we could add a comment, either # Alternatively, an enum is available in place of this string value or # Using a string as no enum is currently available. In the latter case, an issue could be raised. Why I'm commenting this is that, as a reader, I am not sure of the reason for using strings here, but I would take some (avoidable) time to try using enums in their place.
| discretization_scheme["k"] = "second-order-upwind" | ||
| discretization_scheme["epsilon"] = "second-order-upwind" | ||
| initialization = Initialization(solver) | ||
| initialization.defaults.k = 0.000001 |
There was a problem hiding this comment.
| initialization.defaults.k = 0.000001 | |
| initialization.defaults.k = 1e-6 |
examples/00-fluent/battery_pack.py
Outdated
| # Define constants | ||
| # ---------------- | ||
|
|
||
| S = 1 / ohm # a Siemen |
There was a problem hiding this comment.
| S = 1 / ohm # a Siemen | |
| S = 1 / ohm # = 1 siemens |
When surnames are used as units the name is always lowercase whereas the symbol is always uppercase. In this case, the unit is the siemens.
The library should probably expose it directly as a common unit.
There was a problem hiding this comment.
Just found this online
“Siemens” is the singular and plural; “1 siemen” is wrong.
examples/00-fluent/battery_pack.py
Outdated
| graphics.picture.x_resolution = 650 # Horizontal resolution for clear visualization | ||
| graphics.picture.y_resolution = 450 # Vertical resolution matching typical aspect ratio | ||
|
|
||
| all_walls = mesh.surfaces_list.allowed_values() |
There was a problem hiding this comment.
What do you think about adding all() for string types as an alias for allowed_values(), with the intention to deprecate the latter over time? This would align with the new <NamedType>.all() (albeit the semantics mismatch slightly) and eliminate a relatively verbose method name.
There was a problem hiding this comment.
I like that yes, will update the PR soonish
| e_material = SolidMaterial.create(solver, name="e_material", chemical_formula="e") | ||
| e_material.thermal_conductivity = 20 * W / (m * K) | ||
| e_material.uds_diffusivity.option = "defined-per-uds" | ||
| e_material.uds_diffusivity.uds_diffusivities["uds-0"] = ( | ||
| 1e6 * S / m | ||
| ) # Electronic conductivity | ||
| e_material.uds_diffusivity.uds_diffusivities["uds-1"] = ( | ||
| 1e6 * S / m | ||
| ) # Ionic conductivity |
There was a problem hiding this comment.
I have a couple of general questions about this section, i.e. as it already exists, and therefore not directed at @Gobot1234:
1. Terminology:
The quantities provided under uds_diffusivity have units of S/m (electrical conductivity) rather than any form of diffusivity (m²/s).
Is the term “diffusivity” here only a consequence of re-using the generic UDS diffusion operator internally? From a user perspective this is confusing, as the physical meaning is conductivity rather than diffusivity. The API user is not interested in Fluent's implementation details.
2. API structure:
The keys "uds-0" and "uds-1" appear to be created implicitly inside the uds_diffusivities container, with their semantics defined only by comments (electronic vs ionic conductivity).
This raises a few concerns:
- these objects are not discoverable in the public schema/docs;
- their meaning is position-based rather than name-based;
- nothing prevents them being swapped accidentally.
It would obviously make sense to expose a more explicit structure with named fields such as electronic_conductivity and ionic_conductivity, so that the API reflects the physical intent rather than relying on implicit indices?
There was a problem hiding this comment.
You'll see a similar sort of index based API with the multiphase stuff which I also wasn't a big fan of
There was a problem hiding this comment.
@Gobot1234 It would be more defensible if the names weren't utterly meaningless. It shows you the level of thinking that goes into some of this stuff.
There was a problem hiding this comment.
Yeah... as someone with just the code you wouldn't even know what a UDS was which I think is a pretty massive naming failure
3 participants
Context
The examples are currently not following best practices as described by @seanpearsonuk now they do (I hope).
They should also type check better once all the PRs for these things are merged (units, vis and pyfluent), there are a couple of examples which locally have less that 10 issues (I copied over the typing changes from the aformentioned libraries and turned off a couple of checks that currently haven't been fixed) which is a massive improvement IMO
Mostly untested and not currently functional due to various settings API and other github side pull requests not being merged or fixed
Change Summary
create()andget()methods that are more clear in intentpathlibinstead ofos.pathlanuch_fluentto it's appropriateModeClass.from_installcreate()and use that to apply next variable assignments if appropriate rather than performing another lookupname()method though. In practice though i don't like this and we should be instead passing round the full objects e.g.custom_field_function_name=omega_r.name()whereomega_ris acustom_field_function. I think it makes more sense to pass a str | custom_field_function to a parameter calledcustom_field_functionthan passing the same type tocustom_field_function_namewhich would work if you use another one of my PRs)Rationale
We want the examples to be the best things for customers to learn from
NB I've updated some of the examples @MohammedAnsys has PRs open for that looked like they were ready for merging
NBB I 've not switched the save locations for all these to examples_path, though that might be a good idea in the future