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

Infer shape of source space from target space #12

Merged
merged 3 commits into from Jun 8, 2022

Conversation

kclamar
Copy link
Contributor

@kclamar kclamar commented Jun 8, 2022

Hi,

I'm wondering if it's possible to allow transformations of coordinates when the target space has a shape but the source space doesn't?

@kclamar kclamar marked this pull request as ready for review June 8, 2022 07:07
@kclamar kclamar marked this pull request as draft June 8, 2022 07:09
@vigji
Copy link
Member

vigji commented Jun 8, 2022

Hi @kclamar, thanks for the PR!
My concern there is that it can be dangerous to infer source from target in the eventuality one just forgot to define the source shape. Can you make a use case for this? In general, I was assuming one would always have the shape of both, but I am happy to discuss it with a use case at hand!
If we go for this option, I would at least make it not the default behavior - ie, add a kword argument infer_source_shape=False that needs to be passed for the behavior proposed in the PR

@kclamar
Copy link
Contributor Author

kclamar commented Jun 8, 2022

Hi @vigji,

Thanks for the response! In my workflow I save the coordinates of points in the "sal" space and sometimes I need to convert the coordinates to the space of the mpin_zfish_1um atlas. When I initialize the source space for the conversion I need to calculate the shape of the atlas stack after transforming to the source space. I think it'd be convenient to have this new feature since the atlas space already contains the shape information. I also see why it could be dangerous, so I think that having a keyword argument for this feature and making it not the default behavior would be a good option.

Copy link
Member

@vigji vigji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!
This is good to go for me, would you mind just adding a little test for your addition in the tests module so that coverage is not decreased? It should be easy by looking at the rest of the tests in the module, but I can give more pointers if needed!

@kclamar
Copy link
Contributor Author

kclamar commented Jun 8, 2022

I've added a test with axes flipping and reordering.

@kclamar kclamar marked this pull request as ready for review June 8, 2022 17:58
@vigji
Copy link
Member

vigji commented Jun 8, 2022

Awwwwwesome! Thank you very much @kclamar !

@vigji vigji merged commit e8ccc25 into brainglobe:master Jun 8, 2022
@vigji
Copy link
Member

vigji commented Jun 8, 2022

will release a new version with this

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

Successfully merging this pull request may close these issues.

None yet

2 participants