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

No default rounding to center method #256

Open
johnnychen94 opened this issue Aug 19, 2021 · 2 comments
Open

No default rounding to center method #256

johnnychen94 opened this issue Aug 19, 2021 · 2 comments
Milestone

Comments

@johnnychen94
Copy link
Member

Working through #242 #251 and #254, I have to admit that I made a design mistake here, instead of current

function center(A::AbstractArray, r::RoundingMode=RoundDown)
    map(axes(A)) do inds
        round(Int, (length(inds)-1)/2, r) + first(inds)
    end
end
centered(A::AbstractArray, cp::Dims=center(A)) = OffsetArray(A, .-cp)

A better approach is perhaps to:

function center(A::AbstractArray)
    map(axes(A)) do inds
        length(inds)-1)/2 + first(inds)
    end
end
function centered(A::AbstractArray, cp::Dims=center(A), r::RoundingMode=RoundDown)
    OffsetArray(A, map(x->round(Int, x, r), .-cp))
end

This enables users to get the raw float point center coordinate and thus can be used in other scenarios. For instance, in interpolations, arrays are not required to be placed at the grids points. Also, unlike the previous version, applying rounding mode to centered will not be ambiguous anymore (See #250)

Unfortunately, to support this breaking behavior transition, a major version bump is required. For now, we should just stick to the current integer world. Also, I'm not sure how much people want this change so it's better to leave it open for a while before making any deprecations.

@timholy
Copy link
Member

timholy commented Aug 19, 2021

I'm OK with a breaking change/major version bump if that's what is needed, but I haven't yet dug into this carefully enough to comment. Perhaps I'll focus on the Images transition first and come back?

@johnnychen94
Copy link
Member Author

This isn't something urgent to change. I think it's totally acceptable to wait for other more important calls for breaking change. For now, we can just live in the integer world and keep this issue open until we're ready to break everything.

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