matejak
commented
7 years ago 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.pymodule 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.
Right now the mode parameter (e.g. "reflect", "wrap"...) in transform_img and transform_img_dict was ignored (issue https://github.com/matejak/imreg_dft/issues/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.