Skip to content
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

Api change to let run set_attr and get_attr remotely #287

Merged
merged 7 commits into from
Aug 22, 2023

Conversation

mariostieriansys
Copy link
Collaborator

@mariostieriansys mariostieriansys commented Aug 17, 2023

The API now runs set_attr and get_attr directly in EnSight, without looping over the ensobjlist on the PyEnSight side

I computed some performance against a 10k part synthetic dataset:

new branch

same session

session.ensight.objs.core.PARTS.set_attr("SELECTED", True): 263.00 s
session.ensight.objs.core.PARTS.get_attr("VISIBLE", True): 3.70 s

different sessions

session.ensight.objs.core.PARTS.set_attr("SELECTED", True): 243.37 s
session.ensight.objs.core.PARTS.get_attr("VISIBLE", True): 255.68

Main

same session

session.ensight.objs.core.PARTS.set_attr("SELECTED", True): 347.32 s
session.ensight.objs.core.PARTS.get_attr("VISIBLE", True): 82.43 s

different sessions

session.ensight.objs.core.PARTS.set_attr("SELECTED", True): 344.86 s
session.ensight.objs.core.PARTS.get_attr("VISIBLE", True): 344.98 s

So, apart from an overall 30% performance improvement, you also get a huge boost on following operations on the same list. This is because the code is now building an ensobjlist on the EnSight side. So, in the following operations, EnSight will create much faster the same list, using the cached data from the previous computation

@kecolburn
Copy link
Collaborator

I appreciate being included in the review. However, I don't know enough about the internal implementation and execution of this logic to make any sensible critique.

kecolburn
kecolburn previously approved these changes Aug 17, 2023
@mariostieriansys mariostieriansys marked this pull request as ready for review August 18, 2023 10:19
@codecov-commenter
Copy link

codecov-commenter commented Aug 18, 2023

Codecov Report

Merging #287 (6bb8560) into main (014ee3e) will decrease coverage by 0.16%.
The diff coverage is 64.00%.

@@            Coverage Diff             @@
##             main     #287      +/-   ##
==========================================
- Coverage   82.64%   82.49%   -0.16%     
==========================================
  Files          17       17              
  Lines        2391     2399       +8     
  Branches      413      419       +6     
==========================================
+ Hits         1976     1979       +3     
+ Misses        275      274       -1     
- Partials      140      146       +6     
Files Changed Coverage Δ
src/ansys/pyensight/core/listobj.py 75.94% <64.00%> (-4.34%) ⬇️

margalva
margalva previously approved these changes Aug 18, 2023
Copy link
Collaborator

@margalva margalva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved with a suggestion (that applies to both methods). Up to you to take it or leave it.

src/ansys/pyensight/core/listobj.py Outdated Show resolved Hide resolved
randallfrank
randallfrank previously approved these changes Aug 21, 2023
@mariostieriansys mariostieriansys enabled auto-merge (squash) August 22, 2023 07:52
@mariostieriansys mariostieriansys merged commit 2306c2a into main Aug 22, 2023
19 checks passed
@mariostieriansys mariostieriansys deleted the mostieri/set_attr_remote branch August 22, 2023 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants