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

PR: Improve performance of SpragueInterpolator #1268

Merged
merged 3 commits into from
Jun 20, 2024

Conversation

tjdcs
Copy link
Contributor

@tjdcs tjdcs commented May 29, 2024

SpragueInterpolator is the default interpolator for SpectralDistribution making it a highly frequented code path. This PR greatly improves the performance of interpolation by using vectorized code and numpy.

Other interpolators could be similarly sped up but they are lower priority.

@tjdcs tjdcs requested a review from KelSolaar May 29, 2024 00:32
@tjdcs
Copy link
Contributor Author

tjdcs commented May 29, 2024

Two tests are failing

=========================== short test summary info ============================
FAILED colour/colorimetry/generation.py::colour.colorimetry.generation.sd_gaussian
FAILED colour/colorimetry/generation.py::colour.colorimetry.generation.sd_gaussian_fwhm
===== 2 failed, 3464 passed, 11 skipped, 190 warnings in 185.65s (0:03:05) =====

However, my local environment passes these... :/ sneaky. Will investigate.

@tjdcs
Copy link
Contributor Author

tjdcs commented May 29, 2024

@KelSolaar the failing tests are doctest(s) in generation.py. Do you have a prefered resolution? Should I add ellipses... or round... or something else? Seems like the explicit tests are passing within the tolerance we expect / use.

@KelSolaar KelSolaar changed the title Improve performance of SpragueInterpolator PR: Improve performance of SpragueInterpolator May 31, 2024
@KelSolaar
Copy link
Member

Hey @tjdcs,

Thanks a lot! Yeah ellipsis would be absolutely fine here.

Cheers,

Thomas

Copy link
Member

@KelSolaar KelSolaar left a comment

Choose a reason for hiding this comment

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

LGTM besides the einsum usage, I think we need to get rid of the other usages in the codebase.

Comment on lines 1204 to 1209
# Fancy vector code here... use underlying numpy structures to accelerate
# parts of the linear algebra.

y = r[i] + np.einsum(
"ab,ab->b", a.reshape(5, -1), X ** np.arange(1, 6).reshape(-1, 1)
)
Copy link
Member

Choose a reason for hiding this comment

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

With the desire to move toward data-array API compliance and because Einstein Sum is not planned to be supported, we should try to rewrite this using lower level APIs: https://data-apis.org/array-api/latest/extensions/linear_algebra_functions.html

Copy link
Member

Choose a reason for hiding this comment

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

I removed all usages of np.einsum in develop.

@tjdcs tjdcs force-pushed the performance/interpolation branch from ea330ed to 6500280 Compare June 16, 2024 08:19
@tjdcs tjdcs force-pushed the performance/interpolation branch from 9dfdc08 to 94f34ae Compare June 16, 2024 08:53
@tjdcs
Copy link
Contributor Author

tjdcs commented Jun 16, 2024

@KelSolaar looks like these are all passing now. Should be g2g when the last few tasks finish.

@coveralls
Copy link

Coverage Status

coverage: 99.978%. remained the same
when pulling 94f34ae on performance/interpolation
into 9fe1fdf on develop.

@tjdcs tjdcs mentioned this pull request Jun 20, 2024
7 tasks
@KelSolaar KelSolaar merged commit b2da242 into develop Jun 20, 2024
26 checks passed
@tjdcs tjdcs deleted the performance/interpolation branch June 20, 2024 05:48
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.

None yet

3 participants