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

Inconsistent behaviour for applyeager concerning indices #24

Open
Evizero opened this issue May 13, 2018 · 4 comments
Open

Inconsistent behaviour for applyeager concerning indices #24

Evizero opened this issue May 13, 2018 · 4 comments
Labels
Milestone

Comments

@Evizero
Copy link
Owner

Evizero commented May 13, 2018

I just noticed sometimes that the fallback gets rid of arbitrary indices while some implementations do not (and instead return a OffsetArray). This should be streamlined

@Evizero Evizero changed the title Inconsistent behaviour for applyeager converning indices Inconsistent behaviour for applyeager concerning indices May 13, 2018
@johnnychen94
Copy link
Collaborator

johnnychen94 commented Dec 19, 2019

we could add a plain_array to the result of applyeager so that we always get a plain Array. Since it's in eager mode, I believe this is acceptable.

function applyeager(op::Operation, img::AbstractArray, param)
maybe_copy(applylazy(op, img, param))
end

function applyeager(op::Operation, img::AbstractArray, param) 
-     maybe_copy(applylazy(op, img, param))
+     plain_array(maybe_copy(applylazy(op, img, param)))
end 

@johnnychen94 johnnychen94 added this to the v0.6 milestone Dec 19, 2019
@Evizero
Copy link
Owner Author

Evizero commented Dec 19, 2019

I considered this I am pretty sure. The issue is that the augment function is free to choose if an operation is applied eagerly or lazily. So the indices in eager should match the indices in lazy.

As I see it, propagating arbitrary indices are a feature and not a bug

@johnnychen94
Copy link
Collaborator

So the indices in eager should match the indices in lazy.

Cool, I see your point here.

@Evizero
Copy link
Owner Author

Evizero commented Dec 19, 2019

So the indices in eager should match the indices in lazy

That said. That statement is currently not true (if i remember correctly).
So technically your first label was appropriate for the issue 🐛
I just consider it a bug for a different reason 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants