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

Improve Continuous scatter-plot tetrahedron classification #920

Merged
merged 1 commit into from
Apr 13, 2023

Conversation

Lgt2x
Copy link

@Lgt2x Lgt2x commented Apr 12, 2023

Thanks for contributing to TTK!

Before submitting your pull request, please:

  • Review our Contributor Guidelines, in particular regarding code formatting (with clang-format) and continuous integration.

  • Please provide a quick description of your contributions below:

Following similar work on the VTK-m libary, improve the continuous Scatterplot implementation of Shirley-Tuchman tetrahedron classification. This PR also does variable renaming for maintainability.

For each tetrahedron, instead of 4 barycentric coordinates and 3 segment intersections computations in the worst case, all we do now is 4 single-coordinate cross products. The full explanation is provided in the code comments.

This change allows an estimated 20% speedup on the filter execution, rendering non included.

@Lgt2x
Copy link
Author

Lgt2x commented Apr 12, 2023

ping @CharlesGueunet for review :)

@CharlesGueunet
Copy link
Contributor

+1 after small question

Copy link
Collaborator

@julien-tierny julien-tierny left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this PR !
May that be the first of many :)

That's great stuff. I only observed a 9% speedup on my end but that's still good to take!!!

Could you please address the few remarks I left in the code?
Thanks!

@Lgt2x Lgt2x force-pushed the csp-improvements branch from 82af3c4 to 377b770 Compare April 13, 2023 07:19
@julien-tierny
Copy link
Collaborator

alright, it's all good to me :)
@CharlesGueunet anything to add regarding code review?

random question:
the speedup on my desktop -- a Xeon E5-2630 CPU with 12 cores -- is more modest than on my laptop -- an i7-10810U with 6 cores (about 3% VS 9%).
what machine did you do you performance experiments on?

@Lgt2x
Copy link
Author

Lgt2x commented Apr 13, 2023

I tested both on Intel i7-7700HQ (8 cores) and 12th Gen Intel i9-12900K (24 cores) with double-templating enabled and got a more or less 20% improvement.

For this benchmark however, I excluded the rendering part of the algorithm which takes the most time out of all steps especially for large images, to get the speedup only on the core classification & density computation.

@CharlesGueunet
Copy link
Contributor

CharlesGueunet commented Apr 13, 2023

Nothing to add on my side :)
( my small question was addressed directly, and unpublished as I missed one button on github)

@julien-tierny
Copy link
Collaborator

alright, let's go for it then :)
thanks a lot!

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.

3 participants