search

jolars / slopecd

4 stars 2 forks source link

The goal of this PR is to optimize slope-thresholding computation #20

Closed Klopfe closed 2 years ago

Klopfe commented 2 years ago

I tried to implement what we discussed with @jolars by giving a search direction to avoid some useless index search in the loop. It does not seem to improve by much. We still need to investigate.

jolars commented 2 years ago

Good job, hm, I'll take a look tomorrow or early next week. Did you by any chance run a profile before and after?

Klopfe commented 2 years ago

Do we want to merge this new version of slope_threshold?

jolars commented 2 years ago

Do we want to merge this new version of slope_threshold?

Absolutely! Just give me a minute to fix the merge conflicts that I've caused.

Klopfe commented 2 years ago

Do we want to merge this new version of slope_threshold?

Absolutely! Just give me a minute to fix the merge conflicts that I've caused.

No problem, I asked cause I was about to do it but thanks Johan, the solver is looking good :) We are on the right path I believe.

jolars commented 2 years ago

Do we want to merge this new version of slope_threshold?

Absolutely! Just give me a minute to fix the merge conflicts that I've caused.

No problem, I asked cause I was about to do it but thanks Johan, the solver is looking good :) We are on the right path I believe.

Yup, I agree! Do you think we should keep the old thresholding implementation around or should we just drop it?

Klopfe commented 2 years ago

I think, we can drop it. I kept it because I was very unsure of what I was doing and wanted to compare. I think it's allright now and we can always find the old version in old commits.

jolars commented 2 years ago

Do we want to merge this new version of slope_threshold?

Absolutely! Just give me a minute to fix the merge conflicts that I've caused.

More like "give me the better half of a day". I had to debug several small issues that kept cropping up with the updates, but I reckon that it should be good to go now, so I'll merge this one. Just one thing: you had moved get_clusters() to be called for each new epoch, but I've moved it back out (like it was before). Maybe we can open up a new issue to discuss this (if we choose to keep the old approach of not having clusters be updated)?

Klopfe commented 2 years ago

Thanks, @jolars, it looks great! yes, we can discuss this in another PR!