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

Remove allocations in interpolants(grid::RectangleGrid, x::AbstractVector) and interpolate using view #42

Merged
merged 2 commits into from
Dec 14, 2023

Conversation

himanshugupta1009
Copy link
Contributor

  1. Modified the interpolants function to return a view of grid.index and grid.weight instead of slicing it (gets rid of allocations).
  2. Modified the interpolate function to avoid allocation (data[index] was causing allocations).

Performance comparison

Before these modifications

Screenshot from 2023-12-12 15-46-11

After these modifications

Screenshot from 2023-12-12 15-46-43

If there are any issues with it, please let me know. Thanks!

@mykelk mykelk requested review from tawheeler and zsunberg December 13, 2023 00:17
@tawheeler
Copy link
Contributor

Overall this change looks great to me. I'm surprised dot was contributing to allocations, but the data does not lie. Thank you!

Could the indentation of the code be adjusted to match with the existing code? Usually that delta is a matter of tabs vs. spaces.

@himanshugupta1009
Copy link
Contributor Author

@tawheeler I think the allocation was due to the argument that was being passed to the dot function (data[index]).

I didn't realize the indentation mismatch before. Thank you for pointing it out. I have replaced tabs with spaces and it now matches the existing code.

Copy link
Member

@zsunberg zsunberg left a comment

Choose a reason for hiding this comment

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

Looks good to me! There might be a better way to do the for loop, e.g. with sum, but I think we should just merge this!

Copy link
Member

@zsunberg zsunberg 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 a possible alternative for the for loop

Comment on lines +162 to +166
v = 0.0
for (i,data_ind) in enumerate(index)
v += data[data_ind]*weight[i]
end
return v
Copy link
Member

Choose a reason for hiding this comment

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

Would this work?

Suggested change
v = 0.0
for (i,data_ind) in enumerate(index)
v += data[data_ind]*weight[i]
end
return v
return dot(view(data, index), weight)

Copy link
Contributor Author

@himanshugupta1009 himanshugupta1009 Dec 14, 2023

Choose a reason for hiding this comment

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

@zsunberg Yes, this works too. It just slows down the computation by around 15 ns (checked multiple times). If that is acceptable, I can make this change. Let me know.

Before making the change suggested by Dr. Sunberg

Screenshot from 2023-12-13 20-49-49

After making the change suggested by Dr. Sunberg

Screenshot from 2023-12-13 20-49-55

We can also use the sum function like you mentioned but it also causes a similar increase in total time.

sum( i->data[index[i]]*weight[i], 1:length(index))

Copy link
Member

Choose a reason for hiding this comment

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

Either one is fine. dot is easier to read, but the for loop performs slightly better.

@mykelk mykelk merged commit b0ba547 into sisl:master Dec 14, 2023
@zsunberg zsunberg mentioned this pull request Jul 11, 2024
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.

4 participants