Remove some arguments in make_focal_grid()

[spbos]

Hi all,

Currently, when generating a grid with the make_focal_grid() function, the user can set the spatial resolution by either using the 'spatial_resolution' argument or the 'pupil_diameter' + 'focal_length' + 'reference_wavelength' arguments. The latter three arguments only save the user one line of code, namely 'spatial_resolution = lambda * f / D'. While adding 9 lines of code to the function, making it harder to read and understand.

Therefore, the suggestion is to remove the arguments 'pupil_diameter', 'focal_length' and 'reference_wavelength'. To make sure that the user knows what is expected, it should be clear in the documentation how to calculate the spatial resolution (which is already the case).

What do you think?

[ivalaginja]

While I think you're right that removing these would make the function cleaner, I believe having the option of either providing the spatial_resolution or the set of the three other arguments gives the user (even if it seems trivial) a choice that might actually be super useful depending on the used-case of your own scripts. Especially since Emiel said that you guys are putting in effort in setting up some tutorials to teach HCI and optics, keeping both options in there makes make_focal_grid() a little more intuitive to use. This is especially true if you consider that the whole design of hcipy is based on the concept of Fields and Grids, which is a new thing when comparing to other high contrast python libraries, so I would support a certain level of intuition and choice when it comes to function calls. This is also what I personally think sets hcipy apart from other packages like poppy - while both of them are obviously correct and poppy is more mature due to the way longer lifetime of the project, one of hcipy's strong sides is its close tie and enforcement of physical concepts, rather than focusing on fully integrated instruments and optical elements.

In any case, this is a design choice that needs to be made, trading off between cleanliness from a software side, and intuition and physical versatility on the science side. I am curious to see which way @ehpor is leaning towards.

[ehpor]

Actually I could be persuaded both ways.

Supporting the multiple calling conventions via overloaded functions (a la C++) would be by far the best solution. Python doesn't really support this though. Emulation of overloaded functions is of course possible, but is not really Pythonic, so I didn't choose that route. I went for the current function signature for the reason @ivalaginja mentioned: it allows you to use both calling conventions, making it easier for new people to use HCIPy. Also, when calling the function with keyword-style parameters, ie. make_focal_grid(4, 16, pupil_diameter=4.2, reference_wavelength=500e-9, focal_length=10.2), as is pretty much a necessity in the current implementation, makes it very readable as well.

The reasons outlined by @spbos for changing the signature are not really the reasons for which a change could be warranted. The simplification of library-internal code should never be the driver for changing the interface - the cleanliness of the interface is what matters. Removing the additional arguments would simplify the interface and documentation of this function, making it easier to understand at first glance. It would however force a user to know how to calculate spatial resolution, which is I think a prerequisite for using HCIPy in the first place, so that shouldn't be a valid reason. Also the documentation can help by listing the various ways of calculating the spatial resolution.

An additional consideration for a third option: when the user has an F-number and a wavelength, they need to calculate the spatial resolution manually anyway, ie. make_focal_grid(4, 16, spatial_resolution=F_number * wavelength). Is there a reason for not supporting this calling convention, when it does support calling the function with a (pupil_diameter, focal_length, wavelength) set? It seems to me like the current function signature is a weird compromise, while there should be a clear yes or no decision. This is the reason why I told @spbos to create this issue.

[syhaffert]

I am noticing after the update to the new interface that I rather use the spatial_resolution than the other three parameters. It is a little bit cumbersome to constantly add the full three keywords as opposed to a single spatial_resolution. And as @ehpor mentioned why is there support for one way of calculating and not for the other.

[ivalaginja]

Fair point. This might really indicate that holding on to spatial_resolution only might be the way to go, as long as the documentation is clear, which it is. It might be worth adding a mini-notebook or minimal example in the function documentation explaining both ways of calculating the spatial resolution, to show explicitly what is meant by it.

[ehpor]

Okay, I think it's conclusion time.

I'll add a third option of calling the function: make_focal_grid(q, num_airy, f_number, wavelength). Deciding factor for me: it is still possible to calculate the spatial resolution yourself and call the function with that, if you don't want to use the keywords in the function. I'll also update documentation and add a mini-tutorial on the subject; maybe include it in a tutorial on using physical/normalized units?

[ehpor]

I added the f_number argument in 08d12b1 and appended a description of physical vs normalized units at the end of the broadband PSF tutorial in a455e51.