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

Inference improvements for timeevolution #396

Merged
merged 4 commits into from
Jun 15, 2024
Merged

Inference improvements for timeevolution #396

merged 4 commits into from
Jun 15, 2024

Conversation

amilsted
Copy link
Collaborator

This PR does two things:

  • Uses let blocks around inner function definitions to avoid boxing of closure variables.
  • Uses type parameters for function/callable types in some places to encourage specialization.

User-visible changes should be:

  • It is possible to use more generic callable types in some places where Function types were previously required.
  • Parts of MCWF evolution are stabilized and made allocation free, so MCWF evolution should no longer allocate on each call to the integrand.

@amilsted amilsted requested a review from Krastanov June 12, 2024 20:09
Copy link
Collaborator

@Krastanov Krastanov left a comment

Choose a reason for hiding this comment

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

Looks good to me! If you have the bandwidth, I would suggest adding some tests with @allocated. Good to merge either way.

allocated(f::F) where {F} = @allocated f()
somefunc() = something_with_a_solver...
@test allocated(somefunc) < some_small_number

like done here https://github.com/QuantumSavory/QuantumClifford.jl/blob/c1d115d3ee8a929279b724df1cc63fd00a9b255a/test/test_allocations.jl#L6

@amilsted
Copy link
Collaborator Author

Think indeed I don't think I have the bandwidth to add alloc tests right now, but agree it would be nice to have. An alternative is to test the integrand function for allocs directly at runtime and warn the user? Might be too annoying though.

@amilsted
Copy link
Collaborator Author

amilsted commented Jun 15, 2024

I've realized some pieces of this PR (let blocks where types ought to be inferred) might actually be superfluous and a result of this. Still, they shouldn't hurt us.

@Krastanov
Copy link
Collaborator

My vote would be for this to be merged and an issue to be created (maybe a "good first issue") about creating the allocation benchmarks.

I am fine with the superfluous lets.

For reference to future readers, the link from Ashley is to this slack conversation:

image
image

and these two other issues:

JuliaLang/julia#52635
JuliaLang/FunctionWrappers.jl#30

@amilsted amilsted merged commit b9a88b7 into master Jun 15, 2024
10 checks passed
@Krastanov Krastanov deleted the function_types branch June 15, 2024 03:05
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