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

Investigate why there's a transpose operation that cannot be removed. #411

Closed
adamltyson opened this issue May 2, 2024 · 8 comments
Closed
Labels
bug Something isn't working

Comments

@adamltyson
Copy link
Member

This comment from @matham highlights that there is a transpose that, if removed, causes errors.

In theory cellfinder shouldn't require a specific axis order, so it would be good to know why this tranpose is there.

cc @alessandrofelder

@adamltyson adamltyson added the bug Something isn't working label May 2, 2024
@matham
Copy link
Contributor

matham commented May 3, 2024

I did find the cause, there is another place, here, where it's flipped. And both places have to be fixed to make it work. I ran into it during further pytorch stuff and removing both flips fixed the issue.

@adamltyson
Copy link
Member Author

Thanks @matham, I think I'm missing something though, where is the second flip?

@matham
Copy link
Contributor

matham commented May 3, 2024

Right, because in some places, the assumption seems to be that the order is x, y, e.g. here. Which means, when you pass planes to e.g. the ball filter, it needs to have that order. But I guess there was an assumption that the data comes in y, x, like in the link in the last message. So it has to be flipped to make it work with the filter.

But, if it used a consistent order with arbitrary dim1/dim2, it should not need flipping anywhere. And in my pytorch code that seemed to work ok.

@adamltyson
Copy link
Member Author

Thanks for clarifying, I saw your previous link and it looked correct, because it was normal numpy axis ordering. So it seems like there's a mix of numpy and cartesian axis ordering, and we should decide on one.

Everywhere else in BrainGlobe where we've made this decision, we've gone with numpy, so I propose we do the same here. Any issue @brainglobe/active-devs?

@alessandrofelder
Copy link
Member

so I propose we do the same here.

I agree!

@adamltyson
Copy link
Member Author

This is more complicated than I thought, just because there are just so many references to axes in cellfinder. We can do three things:

  1. Leave it as it is
  2. Just remove the two flips as noted above
  3. Go through the whole codebase and standardise (this looks like at least half a day of work)

Any thoughts?

@alessandrofelder
Copy link
Member

My initial instinct would be to implement 2. soon (it's a concrete, scoped step that will avoid unnecessary computations) and leave 3. for a later time point, as we have more pressing issues in the roadmap?

@adamltyson
Copy link
Member Author

Correcting these two flips doesn't seem to fix anything. Tests fail, and detection seems worse. I've opened #413 to track this properly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants