Skip to content

Rustify N3LO Ad #443

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

Open
wants to merge 29 commits into
base: master
Choose a base branch
from
Open

Rustify N3LO Ad #443

wants to merge 29 commits into from

Conversation

giacomomagni
Copy link
Collaborator

No description provided.

@giacomomagni giacomomagni marked this pull request as draft February 12, 2025 09:22
@felixhekhorn felixhekhorn added the rust Rust extension related label Feb 12, 2025
@felixhekhorn felixhekhorn mentioned this pull request Feb 6, 2025
13 tasks
@felixhekhorn felixhekhorn added the refactor Refactor code label Feb 12, 2025
@felixhekhorn
Copy link
Contributor

Also here is another suggestion for the harmonics module: all functions called "S + positive integer" take as single argument $N$, all others take as single argument the cache - i.e. no messing with passing the correct arguments. (Since we are now in a compiled language I'm no longer worried of nesting things) we could even shift the "raw" S-es into the polygamma module ...

what do you think @giacomomagni @tgiani ?

@giacomomagni
Copy link
Collaborator Author

Also here is another suggestion for the harmonics module: all functions called "S + positive integer" take as single argument N , all others take as single argument the cache - i.e. no messing with passing the correct arguments. (Since we are now in a compiled language I'm no longer worried of nesting things) we could even shift the "raw" S-es into the polygamma module ...

what do you think @giacomomagni @tgiani ?

Sure this require a bit of restructure though...

@felixhekhorn
Copy link
Contributor

Sure this require a bit of restructure though...

I made an attempt in 462f62c - so, do we want to do this? I can take care of the copy&paste if you wish ;-)

I also made some adjustments to make pre-commit happy

@felixhekhorn
Copy link
Contributor

I'm also starting to think, that the log expressions are so complicated (look at lm14m2), that we should cache them - what do you think?

@giacomomagni
Copy link
Collaborator Author

I'm also starting to think, that the log expressions are so complicated (look at lm14m2), that we should cache them - what do you think?

Some of them might not be needed, because the Moch et al. expressions are simpler...
So I wanted to finish the conversion and then polish the harmonic file.

@felixhekhorn
Copy link
Contributor

I made an attempt in 462f62c - so, do we want to do this?

I forgot to mention: the prize that we had to pay for this was

let mut cp1 = Cache::new(c.n() + 1.);
ZETA2 * hSm1 - 5. / 8. * ZETA3 + ZETA2 * LN_2 - g3(&mut cp1)

i.e. we had to create a new cache, which is potentially expensive ... there are in principle two strategies I can imagine to avoid that (if we want to avoid it):

  1. math: i.e. maybe we can express $g_3(N+1)-g_3(N)$, like I was playing in the QED PR - but looking at the notes still laying on my desk, I'm not sure we can simplify this
  2. more interface: we add a Cache::set method, which allows us to fill an entry into the cache. Then I know $g_3(N+1)$ will need $S_1(N+1)$ and that I can derive from $S_1(N)$ (instead of recalculating)

I agree with this comment rust-lang/rustfmt#3350 (comment)
and since it is not in stable I do it by hand.
@felixhekhorn
Copy link
Contributor

Any comment on the cache recreation issue?

Else: is this ready? if yes, let's mark it, thus make the benchmarks run and the we can quickly merge

@giacomomagni
Copy link
Collaborator Author

Else: is this ready? if yes, let's mark it, thus make the benchmarks run and the we can quickly merge

Not yet! I need to add the variation to the kernel. I'll do it asap.

@giacomomagni
Copy link
Collaborator Author

Any comment on the cache recreation issue?

Maybe all these optimisation can be addressed once the full ekore is in place ?
I guess that for the moment having a new cache in these function is fine as it will be mostly empty (filled only with g3 and related harmonics). But probably the math solution is also doable...

@giacomomagni giacomomagni marked this pull request as ready for review February 27, 2025 13:49
Copy link
Contributor

@felixhekhorn felixhekhorn left a comment

Choose a reason for hiding this comment

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

  • if only we had Add regression tests #440 🙈
  • do you have by chance a time (ratio) estimate?
  • on second thought: should we also make w1::S1 take only the cache as argument?

besides these small issues the PR is good to go

mode: u16,
c: &mut Cache,
nf: u8,
n3lo_variation: [u8; 3],
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm torn between

  1. having a uniform interface, i.e. passing everywhere [u8; 7] and
  2. passing only the relevant information, i.e. [u8; 3]

look also to gamma_ns_qed ... let's also ask to @tgiani for a second opinion ... maybe at the top level (i.e. here) I slightly prefer 1.?
for 1. we could also define some constants á là const IDX_VARIATION_GG: usize = 0; etc.

in any case I suggest to have the documentation top-level: for singlet see my other comment and also here we need to have an explicit spell out (be it 1. or second)

@giacomomagni
Copy link
Collaborator Author

Just to have a quick feeling on the spped-up.
Running an eko with an x-grid of 30 points and 2 Q2s, in VFNS I get:

  • Python (numba already compiled):
  • order: (0,0), matching (0,0): 71.05s user 2.86s system 103% cpu 1:11.70 total
  • order: (4,0), matching (2,0): 147.30s user 4.52s system 100% cpu 2:30.35 total
  • Rust :
  • order: (0,0), matching (0,0): 10.76s user 1.59s system 125% cpu 9.809 total
  • order: (4,0), matching (2,0): 18.48s user 1.63s system 114% cpu 17.566 total

So the speed up ratio is more or less the same at both orders.

@felixhekhorn
Copy link
Contributor

in VFNS I get

FFNS I assume? other it would be not fair

@giacomomagni
Copy link
Collaborator Author

giacomomagni commented Mar 4, 2025

FFNS I assume? other it would be not fair

VFNS, recall now we're able to decouple the 2 ptos!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Refactor code rust Rust extension related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants