Should we have an option to take a custom propagator in exact_propagate?

[isildur7]

basically the title. Will add an argument to exact_propagate. The propagator is calculated if the None is provided for that argument.

[diptodip]

Yup, just saw this but I think I asked for exactly the same thing in my review 🙂

[isildur7]

I am doing this, but if we are providing a precalculated a lot of the arguments are unnecessary. Should I try to provide default values for all of them? It would make things a little unsafe because you could forget to provide a value to those args when the propagator is None and get the wrong answer.

[diptodip]

We could just make the arguments default to None and then do a check to see whether propagator is None or whether the other arguments are None, but this can get a little bit awkward since we have to do it for both propagators and plus the default values become kind of weird.

Or we could have a new kernel_propagate function that just accepts a Field and a propagation kernel, and the rest stay how they were. The Propagate element can just use this if the chosen mode is transfer or exact_transfer and compute the appropriate kernel. I'm kind of in favor of this version.

[GJBoth]

Or we could have a new kernel_propagate function that just accepts a Field and a propagation kernel, and the rest stay how they were. The Propagate element can just use this if the chosen mode is transfer or exact_transfer and compute the appropriate kernel. I'm kind of in favor of this version.

Me too - but how often do we need a custom kernel? Isn't it easier to just create a separate multislice_sample instead of modifying propagation?

[diptodip]

You could imagine optimizing a propagation kernel to better match reality in some kind of camera-in-the-loop setup, like in a few holography papers. But then we can also have transfer and exact just calling kernel_propagate with different kernels. So it's basically just a Fourier convolution at this point, but with different padding logic.

[diptodip]

Normally I would actually be in favor of just having each function do its own thing, but since we're in Python anyway...

[isildur7]

Right now both exact_propagate and transfer_propagate are like this:

def exact_propagate(*args) -> Field:
    # Calculate propagator
    #<------ code ------>

    # Propagating field (same for both)
    #<------ code ------>

    # Cropping output field (same for both
    #<------ code ------>

    return field

We can make a unified method like this (or simply add an extra one)

def kernel_propagate( *args, propagator: Union[str, Array] = "transfer",) -> Field:
    # Calculate propagator
    if propagator == "transfer":
        # calculate transfer
    elif propagator == "exact":
        # calculate exact
    else:
        propagator = propagator

    # Propagating field (same for both)
    #<------ code ------>

    # Cropping output field (same for both
    #<------ code ------>

    return field

I don't know if Jax likes this. It isn't very pretty either, but it's python. 🤷🏼‍♂️

[diptodip]

This is almost what I'm suggesting, with a new kernel_propagate function. The functions we already have (exact_propagate, transfer_propagate) just calculate the propagator with the appropriate function, and then call kernel_propagate since they are both doing a convolution with the exact same logic, as you pointed out. I want kernel_propagate to just expect that you're going to pass in a propagator --- without creating it. We'll leave that to the original functions. I'm actually doing this right now, so you'll see it soon.

[diptodip]

Alright, take a look at this PR: #80

This updates propagation and the thick sample. I'll merge this in tomorrow. I'm not really anticipating major design issues with this but let me know what you all think.