-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Lazify various calls to all() by using generators instead of comprehensions #1829
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
Conversation
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.
Thanks @eumiro for this PR! And apologies for the delayed review.
I am generally okay with this PR, but a few changes of the changes need to be reverted in order to preserve compatibility pandas Series objects. Details below.
@eumiro you'll see test failures in unrelated code. Those are from a scipy issue, and you may ignore them. |
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.
Thanks @eumiro!
This is another example of a PR where I have to go to the code changes to get any idea of what its purpose is and even then I only half understand it. I recommend putting descriptions in PR's and/or associated issues. |
A jargon glossary would be helpful. “Lazy” means evaluated at the last possible conceivable instant, in this case because |
I understand the concepts actually, just think if it's worth the trouble to make the change it should be worth a few sentences from the contributor to describe it. The benefit of a generator for two simple items must surely be very small. |
I agree that the benefit of this change is minor at best, but as long as the PR was already submitted I didn't see any harm in merging it. I wouldn't suggest anyone spend time on these kinds of changes in the future though :) I also agree that a bit of explanation would have been useful here. I did edit the title to be a bit more helpful but did not think to add explanation in the body. I will do that now! |
[ ] Closes #xxxxdocs/sphinx/source/reference
for API changes.docs/sphinx/source/whatsnew
for all changes. Includes link to the GitHub Issue with:issue:`num`
or this Pull Request with:pull:`num`
. Includes contributor name and/or GitHub username (link with:ghuser:`user`
).remote-data
) and Milestone are assigned to the Pull Request and linked Issue.This simple PR converts several list comprehension expressions into generator expressions. The benefit is that generators are in theory more CPU- and memory-efficient, for example by allowing short-circuiting when passed into a function like
all()
. None of the cases touched in this PR are likely to make much of a practical difference, but if nothing else, shifting the pvlib style more towards generators is probably a good move in the long run.