-
Notifications
You must be signed in to change notification settings - Fork 134
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
FEAT: Add transient support #4990
Conversation
Thanks for opening a Pull Request. If you want to perform a review write a comment saying: @ansys-reviewer-bot review |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4990 +/- ##
==========================================
+ Coverage 79.20% 83.50% +4.30%
==========================================
Files 122 122
Lines 54938 54941 +3
==========================================
+ Hits 43511 45881 +2370
+ Misses 11427 9060 -2367 |
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. Just few suggestions. Also what will happen if the timestep provided is not in the solution timestep ?
@@ -1111,6 +1105,8 @@ def evaluate_faces_quantity( | |||
Dictionary of parameters defined for the specific setup with values. The default is ``{}``. | |||
ref_temperature: str, optional | |||
Reference temperature to use for heat transfer coefficient computation. The default is ``""``. | |||
time : str |
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.
time : str | |
time : str, optional |
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.
You're right. I should've added optional
. I'll add that in another PR.
@@ -5710,6 +5711,8 @@ def add_calculation( | |||
ref_temperature : str, optional | |||
Reference temperature to use in the calculation of the heat transfer | |||
coefficient. The default is ``"AmbientTemp"``. | |||
time : str |
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.
time : str | |
time : str, optional |
for t in ["0s", "1s", "2s", "3s", "4s", "5s"]: | ||
fs.add_calculation("Object", "Surface", "Box1", "Temperature", time=t) | ||
df = fs.get_field_summary_data(pandas_output=True) | ||
assert not df["Mean"].empty |
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.
can we add also assert len(df) == 6 to ensure that all timesteps are added in the calculation. Also what will happen if the timestep provided is not in the solution timestep ?
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.
You'll get an error raised from AEDT. I didn't implement any sanity check (neither the possibility to add all time steps) as there is no way (from APIs) to get saved times. We could read this from setup props, but I don't think it's worth it. It can cause more problems than solutions.
No description provided.