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

adding functions for 2p alignment #7

Merged
merged 3 commits into from
Jun 30, 2020
Merged

adding functions for 2p alignment #7

merged 3 commits into from
Jun 30, 2020

Conversation

hlavian
Copy link
Contributor

@hlavian hlavian commented Jun 26, 2020

Added the old 2p alignment function to fimpy:

  1. The align_2p_volume and _align_and_shift functions were added to fimpy/pipline/alignment.
  2. I made small bug fixes so now it fits the new fimpy and split_dataset modules.
  3. These functions now use the register_translation from skimage and not from fimpy.
  4. I added a new file named plane to fimpy/alignment and copied there the align_single_planes_sobel function.

Copy link
Member

@vilim vilim left a comment

Choose a reason for hiding this comment

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

Good! A test would be nice though :)

@@ -8,6 +8,8 @@
shift_stack,
sobel_stack,
)
from skimage.feature import register_translation
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, I missed this. I switched the functions.

shape_block=(dataset.shape_block[0], 1) + dataset.shape_block[2:],
)

if verbose:
Copy link
Member

Choose a reason for hiding this comment

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

we should replace all of those verbose with the logging module (can be done in a later PR)

"""

# prepare the destination
new_dataset = EmptySplitDataset(
Copy link
Member

Choose a reason for hiding this comment

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

if resolution is in the original dataset, it would be good to pass it here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I add an optional input variable for the resolution (with None as default)?

Copy link
Member

Choose a reason for hiding this comment

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

no, it doesn't make sense to insert it here, it should only be passed if the original dataset has it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, so this request is not clear to me. What does if the original dataset has it means? I thought because fimpy is public it should be general.

Copy link
Member

Choose a reason for hiding this comment

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

So at some point it was decided that the SplitDataset class optionally contains resolution and dimension order attributes
e.g.
https://github.com/portugueslab/split_dataset/blob/1faa37954a1fe7bcd23021e2f93238f7b94affb5/split_dataset/split_dataset.py#L113
it makes sense that all operations that work on SplitDataset preserve this information. (so if you e.g. align a dataset which has these fields, the aligned dataset will have these attributes as well). A more elegant, consistent solution for all of this would use xarray, but @vigji is still ambivalent over file formats supported by xarray.

Copy link
Member

Choose a reason for hiding this comment

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

I am ambivalent indeed :D so so far we are just storing the resolution, and if one day we move to xarray supporting formats we have the information to move there the stack easily

Copy link
Member

Choose a reason for hiding this comment

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

it's not just resolution, dimension order is important too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can I merge the branches and deal with this later? I would like to start running alignment of new data

@hlavian hlavian merged commit 3d50222 into master Jun 30, 2020
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.

3 participants