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

Use builtin methods on lists #1316

Merged
merged 2 commits into from
Oct 24, 2024
Merged

Conversation

arunkannawadi
Copy link
Member

@arunkannawadi arunkannawadi commented Oct 24, 2024

I have been spending some time this week hacking GalSim and noticed a few places where builtins are known to be faster than the numpy equivalents generally. This is not likely to move the needle in terms of the overall efficiency, but this is something I had to do anyway during the hack and I thought I might as well try to get it merged here.

@arunkannawadi arunkannawadi marked this pull request as ready for review October 24, 2024 05:14
Copy link
Member

@rmjarvis rmjarvis left a comment

Choose a reason for hiding this comment

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

This is fine I guess. But the length of these lists is typically 2 or 3. So the time difference between any implementation of these operations is basically zero. It's completely irrelevant to any efficiency concern.

There were a number of places where library was misspelt. This commit
fixes those typos.
@arunkannawadi arunkannawadi force-pushed the u/arunkannawadi/builtins-on-lists branch from 8a48933 to 82feef0 Compare October 24, 2024 14:17
@arunkannawadi
Copy link
Member Author

Oh, I don't doubt that one bit. These are some changes that I had to do anyway to get my hacky version running. Btw, I'm not in any hurry and this need not result in a patch release immediately.

@rmjarvis rmjarvis merged commit 3cb6e79 into main Oct 24, 2024
10 checks passed
@rmjarvis rmjarvis deleted the u/arunkannawadi/builtins-on-lists branch October 24, 2024 15:31
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.

2 participants