Skip to content

Task/ie 8/ie 13/flip image - #8

Open
DhimanGhosh wants to merge 12 commits into
mainfrom
Task/IE-8/IE-13/Flip_Image
Open

Task/ie 8/ie 13/flip image#8
DhimanGhosh wants to merge 12 commits into
mainfrom
Task/IE-8/IE-13/Flip_Image

Conversation

@DhimanGhosh

@DhimanGhosh DhimanGhosh commented May 6, 2022

Copy link
Copy Markdown
Owner

This pull request is linked with the issue: IE-13

Description:
Added support for flipping the image horizontally or vertically

Checklist:

  • Link feature / issue to pull-request
  • Link pull-request to feature / issue
  • Develop feature / issue
  • CHANGELOG updated
  • Implement unit tests if possible
  • Testing feature / issue
  • Wheel test done
  • Assign yourself to the pull-request
  • Documentation

@DhimanGhosh DhimanGhosh self-assigned this May 6, 2022
@DhimanGhosh
DhimanGhosh requested a review from Jaidip1994 May 6, 2022 22:44
@DhimanGhosh DhimanGhosh added this to the image_editor-0.0.2 milestone May 6, 2022
@DhimanGhosh DhimanGhosh added the enhancement New feature or request label May 6, 2022
@DhimanGhosh
DhimanGhosh marked this pull request as ready for review May 6, 2022 23:10

@Jaidip1994 Jaidip1994 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Don't remove these files output/sample_image/OPIMG_1.png this will not check-in output file directory.
I have added a gitignore file in this commit 8b518dd

Comment thread .gitignore
Comment on lines +14 to +17
# virtualenv
env/
venv/
whlenv/ No newline at end of file

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@DhimanGhosh can we have like *env ? then all the env files folders will be
And do we really need to have this as we have this already handled?
image

So a blank folder will never be checked in

Comment thread image_editor/__init__.py
Comment on lines +2 to +4
RED_CHANNEL = 0
GREEN_CHANNEL = 1
BLUE_CHANNEL = 2

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can this be done like this

class ColorChannel:
    Red, Green, Blue = range(3)

Similarly for the other two different enum classes?
https://stackoverflow.com/a/702903

Comment thread image_editor/__init__.py
Comment on lines +7 to +14
CROP_CUSTOM = 0
CROP_1_1 = 1
CROP_CIRCLE = 2

# Flip Directions
FLIP_VERTICAL = 0
FLIP_HORIZONTAL = 1
FLIP_VERTICAL_HORIZONTAL = 2

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Same review as above

from image_editor.core.image_processing.interface.Image_Processing_Abstract import ImageProcessingAbstract
from image_editor.utils.convert_image import image_to_numpy_array, numpy_array_to_image, transparent_background
from glob import glob
from image_editor.utils.convert_image import image_to_numpy_array, numpy_array_to_image, transparent_background, cv_to_image

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can we follow this coding standard ?
https://stackoverflow.com/a/29193752
from module import xx still occurs sometimes in my code. I use it in cases where even the as format appears exaggerated, the most famous example being from datetime import datetime (but if I need more elements, I will import datetime as dt).

Suggestion would be like this import image_editor.utils.convert_image as ieuc

new_img[:, :, [1, 2]] = 0
if channel.lower() == 'g':
if channel == ie.GREEN_CHANNEL:
new_img[:, :, [0, 2]] = 0

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is this correct? because we need just one color channel and not two ?
And one more suggestion can the ENUM be designed like a index ?
new_img[:, :, color_channel_enum], then no need of if-else

elif kwargs.get('aspect_ratio', '1:1') == '1:1':
shift = kwargs.get('dimension', 0)
elif aspect_ratio == ie.CROP_1_1:
shift = kwargs.get('shift', 0)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

shift can it be made a constant, and all the expected KWARGS be made as a constant?
like aspect_ratio, amount , direction ...

Comment on lines +109 to +114
if flip_direction == ie.FLIP_VERTICAL:
img = cv.flip(img, 0)
elif flip_direction == ie.FLIP_HORIZONTAL:
img = cv.flip(img, 1)
elif flip_direction == ie.FLIP_VERTICAL_HORIZONTAL:
img = cv.flip(img, -1)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can the enum value be made as a third param to cv.flip

Suggested change
if flip_direction == ie.FLIP_VERTICAL:
img = cv.flip(img, 0)
elif flip_direction == ie.FLIP_HORIZONTAL:
img = cv.flip(img, 1)
elif flip_direction == ie.FLIP_VERTICAL_HORIZONTAL:
img = cv.flip(img, -1)
img = cv.flip(img, flip_direction)

Comment on lines +37 to +39
list_of_files = Path(dirname).glob(f'*.{extension}')
seq_no = max([0] + [int(Path(fn).stem.split('_')[1]) for fn in list_of_files if '_' in Path(fn).stem])
new_file_path = dirname + os.sep + f'OPIMG_{seq_no + 1}.{extension}'

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can this be made common ? as this is used above also

def numpy_array_to_image(image_array, image_path: str, file_extension=None) -> str:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants