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

Miscellaneous improvements #23

Merged
merged 3 commits into from
Nov 18, 2024
Merged

Miscellaneous improvements #23

merged 3 commits into from
Nov 18, 2024

Conversation

gdalle
Copy link
Contributor

@gdalle gdalle commented Nov 10, 2024

In conjunction with my review #21, here are some suggestions that should improve your code from a correctness and performance point of view.
There is still more to optimize if you want to, especially regarding allocations. Let me know if you're interested in pursuing this.

Note that I have not performed benchmarks for lack of a meaningful benchmarking instance, but I'm 90% sure that these changes are beneficial. The only downside I can anticipate is a slightly increased compilation latency.


Correctness

Performance

  • Parametrize field types instead of leaving them abstract. This is especially important for function and container fields. See this documentation section to know more.
  • Standardize option initializations to Float64 when it makes sense and avoid implicit conversions from Int.
  • Explicitly annotate the argument L to force specialization even though it is a function.

Other updates

  • Remove outdated dependency on Parameters.jl and bump Julia compatibility to 1.10 (the current long-term support version). As of 1.10, Parameters.@with_kw can be replaced with the built-in @kwdef, and Parameters.@unpack with the built-in (; a, b, c) = object syntax.

@gdalle gdalle changed the title Improve type-stability Miscellaneous improvements Nov 10, 2024
@mykelk
Copy link
Member

mykelk commented Nov 10, 2024

These are great ideas! Thanks so much!

@codecov-commenter
Copy link

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

@FlyingWorkshop FlyingWorkshop merged commit a364bc7 into sisl:main Nov 18, 2024
4 checks passed
@gdalle gdalle deleted the speedup branch November 18, 2024 22:07
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.

4 participants