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

Speedup C-contiguous arrays #1074

Merged
merged 10 commits into from
Mar 1, 2023
Merged

Conversation

erikvansebille
Copy link
Member

This PR implements the 'engineering speedup' proposed by @CKehl in #957 by making sure data arrays are C-contiguous from the start (so that they don't need to be transposed on the fly)

Copy link
Contributor

@CKehl CKehl left a comment

Choose a reason for hiding this comment

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

Well, just to add the final note to this PR:
For now, I highly discourage those changes of array-copy changes in the main branch (especially before wrapping up the node-list particle set), as it will invalidate any benchmarking already done, and hence make me redo any publishable measurements.

The changes made until now look quite good. I suggest pausing this PR and continuing with it as soon as the performance benchmarks can be easily separated from main again. At that point, the engineering improvements can safely be pursued without invalidating publishable measurements.

@erikvansebille
Copy link
Member Author

Sure, I understand and was not planning to merge this PR before you had completed your benchmarks. I was planning to use this branch to do some benchmarking of its speedup on of my own simulations.

Let's then merge this PR asap (surely before the next Parcels release) so that users can take advantage of it too; would be a shame to deny them this potential speed-up for their simulations!

@erikvansebille erikvansebille merged commit 0b6f17d into master Mar 1, 2023
@erikvansebille erikvansebille deleted the speedup_C-contiguous_arrays branch March 1, 2023 08:28
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