Skip to content

Cache divgcd(::Int8, ::Int8) #9

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

Closed
wants to merge 12 commits into from
Closed

Cache divgcd(::Int8, ::Int8) #9

wants to merge 12 commits into from

Conversation

MilesCranmer
Copy link
Member

@MilesCranmer MilesCranmer commented Jun 7, 2023

This creates cached_divgcd with a specialized method for Int8 which caches all results at compilation time. Can get a 4x speedup on vectorized computations with normal Rational{Int8}.

This defines a CRational struct that uses this cached_divgcd for relevant rational calculations

@oscardssmith

@github-actions
Copy link
Contributor

github-actions bot commented Jun 7, 2023

Benchmark Results

main a86d431... t[main]/t[a86d431...]
creation/Quantity(x) 4 ± 0.1 ns 3.2 ± 0.1 ns 1.25
creation/Quantity(x, length=y) 3.3 ± 0.1 ns 3.3 ± 0.1 ns 1
time_to_load 0.138 ± 0.0032 s 0.137 ± 0.0024 s 1.01
with_numbers/*real 3.3 ± 0.1 ns 3.2 ± 0.1 ns 1.03
with_numbers/^int 28.4 ± 2 ns 0.0595 ± 0.0028 μs 0.477
with_numbers/^int * real 28.3 ± 2.2 ns 0.0594 ± 0.0026 μs 0.476
with_quantity/*y 17.8 ± 0.4 ns 0.0341 ± 1e-06 μs 0.522
with_quantity//y 17.8 ± 0.4 ns 0.039 ± 0.0002 μs 0.456
with_quantity/^y 27.4 ± 2.1 ns 0.0589 ± 0.0012 μs 0.465
with_self/dimension 1.6 ± 0.1 ns 1.6 ± 0.1 ns 1
with_self/inv 18.5 ± 0 ns 16.5 ± 0 ns 1.12
with_self/ustrip 1.6 ± 0.1 ns 1.6 ± 0.1 ns 1

Benchmark Plots

A plot of the benchmark results have been uploaded as an artifact to the workflow run for this PR.
Go to "Actions"->"Benchmark a pull request"->[the most recent run]->"Artifacts" (at the bottom).

@oscardssmith
Copy link
Collaborator

I've convinced myself that the right type here is actually a Int8/6, since the math then becomes trivial and doing a search has not lead me to any use-cases that need a power that isn't a multiple of 1/6th.

@MilesCranmer
Copy link
Member Author

Wow, check out the speeds here! So caching the gcd is enough to make normal Rational{Int8} fast. Then it is much safer from overflow too, because it always reduces to lowest coprimes.

@MilesCranmer
Copy link
Member Author

I do think the best option would be to parametrize the Dimensions type by this choice. And just have the default be Rational{Int} I can probably get away with Int8/6 as well in my stuff, though I’m sure some people will have some weird system where they need large rational powers… For example, constants associated with a polytropic index in astrophysics can pretty much have any rational power on the units you can write down.

@oscardssmith
Copy link
Collaborator

The type piracy here isn't great but the performance is. One improvement here is that you can make the table 2x smaller by making x be smaller than y (since gcd commutes).

@MilesCranmer
Copy link
Member Author

Surprisingly it is slower. I guess the <(::Int8,: :Int8) incurs more of a penalty than just the lookup?

@MilesCranmer
Copy link
Member Author

@oscardssmith two new changes:

  1. I made a custom CRational type to avoid type piracy. It's an internal type, so I only overload the methods we actually need.
  2. I switched to using SafeInt8 again.

The speed goes down a bit, but it's so much safer than normal Ratios.jl that I think this is worth it.

When you have a chance could you maybe try to get Dimensions{R} working for a user-specified R?

@oscardssmith
Copy link
Collaborator

will do

@MilesCranmer
Copy link
Member Author

Superseded by the faster & cleaner #21

@MilesCranmer MilesCranmer deleted the int8-caching branch July 10, 2023 06:36
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

2 participants