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

ENH: Only call find_spec sometimes #4640

Merged
merged 1 commit into from
Aug 24, 2023

Conversation

matthewturk
Copy link
Member

In some cases, particularly when instantiating lots of child masks, the way the MutableAttribute object is set up could lead to find_spec being called inside some tight loops.

But, the results of find_spec shouldn't change during the course of one interpreter's existence. So we can move the checks to the outside of the __get__ call, which reduces costs of grid indexing considerable in some cases.

Copy link
Member

@neutrinoceros neutrinoceros left a comment

Choose a reason for hiding this comment

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

good catch !

Comment on lines +137 to +140
if display_array and find_spec("ipywidgets") is not None:
self.display_array = True
else:
self.display_array = False
Copy link
Member

Choose a reason for hiding this comment

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

I have a small preference for the one-liner form, but it's not crucial (I've checked that it is black-compliant)

Suggested change
if display_array and find_spec("ipywidgets") is not None:
self.display_array = True
else:
self.display_array = False
self.display_array = display_array and find_spec("ipywidgets") is not None

@neutrinoceros
Copy link
Member

the results of find_spec shouldn't change during the course of one interpreter's existence.

Note that this isn't 100% true: it is possible to install a package at runtime using import pip; pip.main(["install", "pkg"]) or equivalently (and perhaps more commonly), IPython's magic command %pip, but it seems reasonable to me to sacrifice a little flexibility in that exceptional case to save performance in the general case.

@matthewturk
Copy link
Member Author

Yeah, I know it's not strictly true, but it's true enough. We shouldn't bother catching it.

@neutrinoceros
Copy link
Member

agreed

@matthewturk matthewturk added enhancement Making something better performance labels Aug 21, 2023
@chrishavlin
Copy link
Contributor

chrishavlin commented Aug 22, 2023

for a bit of extra context: we found that this find_spec call was sucking up ~20s when calling ds.find_max('density') with some santa fe light cone output -- this PR takes that calculation from 90s down to 70s (on my macbook).

Copy link
Contributor

@chrishavlin chrishavlin left a comment

Choose a reason for hiding this comment

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

Oops, didn't actually hit approve with my last comment :)

@neutrinoceros neutrinoceros added this to the 4.2.2 milestone Aug 24, 2023
@neutrinoceros
Copy link
Member

Moving this to 4.2.2 upon @matthewturk's request

@jzuhone jzuhone merged commit a0a2169 into yt-project:main Aug 24, 2023
12 checks passed
meeseeksmachine pushed a commit to meeseeksmachine/yt that referenced this pull request Aug 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Making something better performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants