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

Minimal API for imageuler #25

Closed
mfschubert opened this issue Mar 15, 2024 · 2 comments
Closed

Minimal API for imageuler #25

mfschubert opened this issue Mar 15, 2024 · 2 comments

Comments

@mfschubert
Copy link
Contributor

In the context of fixes and potential improvements to the imageruler (#24 , #22 , #23 ), it may be worth reconsidering the API. Currently, the API is a bit complex, accommodating e.g. periodic boundary conditions, optional border exclusion for violation detection, padding modes, kernel shapes, and non-square pixels in the top-level functions.

def minimum_length_solid_void(
    array: Array,
    phys_size: Optional[PhysicalSize] = None,
    periodic_axes: Optional[PeriodicAxes] = None,
    margin_size: Optional[MarginSize] = None,
    pad_mode: Tuple[PaddingMode, PaddingMode] = (PaddingMode.SOLID, PaddingMode.VOID),
    kernel_shape: KernelShape = KernelShape.CIRCLE,
    warn_cusp: bool = False,
) -> Tuple[float, float]:
    ...

Some of the previously-identified issues only arise when these features are exercised. In the photonics-opt-testbed (code search), only periodic boundaries are used, and so it seems we could get away with something like,

def minimum_length_solid_void(
    array: Array,
    periodic_axes: Optional[PeriodicAxes] = None,
) -> Tuple[float, float]:
    ...

Aiming for a simpler API should make it easier to develop a well-tested and reliable implementation of the imageruler algorithm. I would be happy to contribute improvements to the codebase, but believe it is probably worth discussing larger changes before submitting a PR. In particular, it would be good to hear if there is a case for retaining the the phys_size, margin_size, pad_mode, and kernel_shape arguments.

@stevengj
Copy link
Contributor

stevengj commented Mar 21, 2024

This makes a lot of sense to me.

At least some of the features can be changed from function parameters to function compositions — e.g. padding can be a separate function, resizing to square pixels can be a separate function.

And some parameters like kernel_shape they only seem to have been needed in the research phase.

@stevengj
Copy link
Contributor

Closed by #30

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

No branches or pull requests

2 participants