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

feat: removed computation of drho from standard windowing procedure. #152

Merged
merged 1 commit into from
Feb 13, 2023

Conversation

fjosw
Copy link
Owner

@fjosw fjosw commented Feb 9, 2023

I recently looked at simulations where I have access to more than a million samples and noticed that the standard windowing procedure can become very slow because most of the time is spent computing the error of rho which is not needed for the windowing but only when visualizing rho as a function of the window. For this reason I propose to remove the computation of drho for the standard windowing procedure. On my machine in a test setup with 5 million samples and $\tau_\mathrm{int}\approx 150$ the gamma method took 20.4s with the previous version and 820ms after the change.

The only downside that I can see is that the error of rho is not available for every data point when calling the plot_rho method. I attached how the output of this method looks like with the changes I propose.

image

@fjosw fjosw requested a review from s-kuberski February 9, 2023 13:08
@s-kuberski
Copy link
Collaborator

Hi! This is certainly some unnecessary work when one does not like to inspect the autocorrelation explicitly. Have you considered pull the method out of gamma_method such that one could call it explicitly, e.g., when plotting rho? Otherwise the user has to go great length to do this themselves. On could do trigger the computation as default or via keyword in plot_rho.

@fjosw
Copy link
Owner Author

fjosw commented Feb 13, 2023

Thanks for the feedback. I thought about the option of calling _compute_drho in plot_rho but thought it takes me to long to implement and test it. I will merge the PR for now but we could come back to this if it is needed.

@fjosw fjosw merged commit ee74ec4 into develop Feb 13, 2023
@fjosw fjosw deleted the feat/speed_up_windowing branch February 13, 2023 16:54
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