-
Notifications
You must be signed in to change notification settings - Fork 52
docs: Added example on the battery cell simulation. #4361
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
Title should be as follows: docs: Added example on the battery cell simulation. This will avoid commit style failure. |
Please refer workflow file and add similar section on the following line: https://github.com/ansys/pyfluent/blob/main/.github/workflows/execute-examples-weekly.yml#L153
This will help to run this example weekly in the GitHub. |
Looks, good. You can try using ansys-fluent-visualization for rendering graphics as well from your side and provide your views on how it works: https://github.com/ansys/pyfluent-visualization |
Hi @prmukherj, Thanks for reviewing the PR. |
…hub.com/ansys/pyfluent into docs/example_on_battery_cell_simulation
else: | ||
raise FileNotFoundError("Remote file does not exist.") | ||
|
||
def download(self, file_name: list[str] | str, local_directory: str | None = None): | ||
def download(self, file_name: list[str] | str, local_directory: str | 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.
@MohammedAnsys The changes in this file are not relevant to this PR. These are old changes. This should be resolved before merging the PR. You can pull latest changes from main branch to resolve this.
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.
@MohammedAnsys This is not resolved properly. Unresolving again.
Not relevant to this PR, since deleting it.
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.
Looks good!
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.
@abhishekchitwar src/ansys/fluent/core/utils/file_transfer_service.py
has been deleted. This should be resolved properly.
cc: @seanpearsonuk
"pyfluent/battery_thermal_simulation", | ||
save_path=os.getcwd(), | ||
) | ||
|
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’ve noticed that comments in this PR are written in the passive voice, which differs from the active style used in other PyFluent examples. The active style is preferred for consistency and readability.
We should also aim for economy in comments—only add them when they communicate something genuinely useful to the reader. Redundant or overly literal comments can slow readers down and may indicate that the script or API itself could be clearer. If many comments feel necessary, that might be a sign the underlying flow or naming could be improved.
mesh["mesh-1"].surfaces_list = surfaces | ||
mesh["mesh-1"].options.edges = True | ||
mesh["mesh-1"].display() |
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.
-
create()
already returns the new object, so you can store it directly and avoid repeatedly indexingmesh["mesh-1"]
. This makes the code cleaner and more object-oriented. -
Setting each property in separate calls is relatively inefficient, especially when each call triggers back-end processing and interprocess communication (which may be remote). Where possible, group property assignments into a single update.
-
In cases where repeated access is unavoidable, aliasing (
mesh_1 = mesh["mesh-1"]
) helps reduce duplication and improves readability.
graphics.views.restore_view(view_name="isometric") | ||
|
||
graphics.picture.save_picture( | ||
file_name="Single_Battery_Cell_Mesh.png", | ||
) |
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.
Does this work for all Fluent run modes?
Aside: I have concerns about our examples being dominated by Fluent graphics usage which is not a first class part of the API and can easily be rendered unusable in a clean headless mode. We need to think about structuring the examples so that users can learn about the full range of alternatives.
battery.zone_assignment.passive_zone = ["tab_nzone", "tab_pzone"] | ||
battery.zone_assignment.negative_tab = ["tab_n"] | ||
battery.zone_assignment.positive_tab = ["tab_p"] | ||
battery.zone_assignment.print_battery_connection() |
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.
This appears to print directly to the console, which makes it more of a debugging or inspection utility than a proper API method. Such functions generally shouldn’t be part of the public API, and we should avoid using them in examples. Examples should demonstrate supported, programmatic workflows that return data rather than only printing it.
# By enabling the Battery Model, Fluent activates the physics and equations needed | ||
# to simulate how the battery behaves electrically, chemically, and thermally. | ||
# | ||
# Select NTGK/DCIR Model: | ||
# This is a simplified but accurate model (based on experimental data) for simulating | ||
# how voltage and heat change during discharge. | ||
# NTGK stands for Newman, Tiedemann, Gu, and Kim, who developed this model. | ||
# | ||
# Set Nominal Cell Capacity (14.6 Ah): | ||
# Defines the total amount of charge the battery can store, | ||
# based on the actual battery specs. | ||
# | ||
# Enable Joule Heat in Passive Zones: | ||
# Includes heat generated in non-active parts like tabs and connectors. This makes the | ||
# thermal results more accurate. | ||
# | ||
# Specify C-Rate (1C): | ||
# Sets how fast the battery discharges. A 1C rate means the battery will fully discharge | ||
# in 1 hour. Higher C-rates mean faster discharge. | ||
# | ||
# Minimum Stop Voltage (3 V): | ||
# Fluent will stop the simulation if the battery voltage drops | ||
# below this. It's a safety cutoff, just like in real batteries. | ||
# | ||
# Assign Zones under Conductive Zones: | ||
# You're telling Fluent which parts are the active battery | ||
# region (electrochemical zone) and which are conductive parts like tabs or connectors. | ||
# | ||
# Assign External Connectors: | ||
# Defines where the current enters and leaves the battery. These are the positive and | ||
# negative terminals. | ||
# | ||
# Print Battery System Connection Info: | ||
# This checks if Fluent understood your setup and shows how everything is connected. | ||
# |
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.
Is a lot of this just telling the reader what the defaults are or is it all actioned somewhere in the script? The comments should make this clearer.
# | ||
# How well each material conducts electricity | ||
# | ||
# This helps fluent calculate voltage, current, and heat correctly during the simulation |
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.
Fluent should always be capitalised, "Fluent"
.
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.
Also, the list is inconsistently formatted: what, How, This.
"density": 8978, | ||
"specific_heat": 381, | ||
"thermal_conductivity": 387.6, | ||
}, |
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 chemical_formula values here ("e"
, "pmat"
, "nmat"
) are placeholders, not actual chemical formulas. We should either use realistic values to avoid confusion, or make it clear in a comment that these are arbitrary identifiers for demonstration purposes only.
}, | ||
] | ||
|
||
solids = solver.settings.setup.materials.solid |
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.
We should aim for direct assignment. The existence of the following code in an example script would only imply that the API is difficult to use.
"density": 2092, | ||
"specific_heat": 678, | ||
"thermal_conductivity": 18.2, | ||
"uds_diffusivity": { |
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.
Are we missing a definition for UDS Diffusivity?
wall["wall_active"].thermal.heat_transfer_coeff.value = 5 | ||
|
||
solver.settings.setup.boundary_conditions.copy( | ||
from_="wall_active", to=["wall_n", "wall_p"], verbosity=True |
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.
Since it's unobvious what the implication of verbosity=True
is, that should be commented here, or preferably the argument should be omitted.
Reason: We need to report the API design in this instance, as once again, this does not belong in the API: verbosity:
"Copy boundary conditions: Print more information."
in other words, the verbosity=True
argument makes this method print directly to the console, which is undesirable for API calls shown in examples (or in general).
Aside: verbose=True
and verbosity=1|2|3
would make more sense.
# Specifying solution settings | ||
# ===================================================================================== | ||
# Since the simulation models a solid battery without fluid flow, | ||
# flow and turbulence equations are disabled, and residual criteria are set to 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.
For none
write "none"
to clearly disambiguate from Python None
.
# | ||
# No flow = No need to solve equations for velocity or pressure. | ||
# | ||
# No turbulence = No air or liquid movement that could become chaotic. |
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.
It isn't made sufficiently clear how this correlates with the following code. The issue is partly due to API design.
|
||
# Surface report for voltage | ||
surface_reports = solver.settings.solution.report_definitions.surface | ||
surface_reports.create("voltage_vp") |
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.
Avoid obscure abbreviations.
graphics.picture.save_picture(file_name="Single_Battery_Cell_5.png") | ||
|
||
# Save case file | ||
solver.settings.file.write(file_name="unit_battery.cas.h5", file_type="case") |
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 realise that the API contains such general methods as read()
and write()
as well as the specific versions. I have a preference for the latter, especially while file_type
is a string argument.
Added End to End PyFluent example on Single Cell Battery Simulation based on Ansys Fluent documentation.
Please review the same!
Thanks!