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

change transform functions to acknowledge boundary mode parameter #28

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

maweigert
Copy link

Right now the mode parameter (e.g. "reflect", "wrap"...) in transform_img and transform_img_dict was ignored (issue #26 ) - this should add the expected behaviour.

I got rid of the extra bigshape padding as it would lead to double reflection/wrapping and I could not find any use case where it would matter (plus, now the transforms behave exactly as expected from ndimage.interpolation) - if there is a reason I am missing to have the bigshape padding, just add the line before again.

@matejak
Copy link
Owner

matejak commented Feb 7, 2017

Hello,
thank you for the contribution.
I can see that there are some issues with the code as it doesn't pass tests. However, we will figure it out as I will have a bit more of time this week.
Before that, I would like to ask you for the following:

  • Please rebase your PR against the current master. This way, we will get rid of some noise (I have incorporated some of the syntax enhancements from your PR into the master).
  • It is great that you have added tests - I suggest that you move them to the utils.py module instead of creating a new one. The unittest approach in imreg_dft is that the unittest tree matches the source tree.
  • Please rewrite your docstrings (great that you have provided them!) from the Numpy style to the Google style (see: ...) as the rest of imreg_dft uses the Google style.
  • Please add your name into the AUTHORS file as part of the pull request in order to claim the well-deserved credit.

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.

2 participants