6
\$\begingroup\$

Context: Based on the code we can find here, an API to control to control Niryo's robots pyniryo/vision/image_functions:l120.

Return orientation of a contour according to the smallest side in order to be well oriented for gripper

This refactoring produces the desired output and is written by me.

I added type narrowing of the return type to radians in order to give more information on the semantic value of the return value, i.e., to prevent further mix with values that are expected to be degree, for example.

I understand that this style or practices might be disgraceful to some, so I'm looking for feedback and arguments against my code so I can improve.

import collections.abc import math import typing import cv2 import numpy as np Radians = typing.NewType("Radians", float) def radians(x) -> Radians: return Radians(math.radians(x)) def get_contour_angle(contour: collections.abc.Sequence[np.ndarray]) -> Radians: _, (width, height), angle = cv2.minAreaRect(contour) near_square_ratio = 1.25 > (width / height) > 0.75 if near_square_ratio and angle < -45: return radians(angle + 90) if near_square_ratio: return radians(angle) if angle > 0: return radians(angle - 90) if width < height: return radians(angle + 180) return radians(angle + 90)

Some argued that the firsts two if should be nested, but I think that the result makes the "what happens" of the top if far from the condition, making it a little bit (I'm not saying "a lot") to contextualize, compared to the flat version.

 if near_square_ratio: if angle < -45: return radians(angle + 90) return radians(angle)

For me, the benefit is too small compared to the mental cost of the pattern.

A ternary expression could be used, or even multiplication of 90 by boolean, but those alternatives don't find their grace into my eyes (yet?).

 if near_square_ratio: return radians(angle if angle >= -45 else angle + 90) # Or if near_square_ratio: return radians(angle + 90 * (angle < -45))
\$\endgroup\$
2

2 Answers 2

Reset to default
7
\$\begingroup\$

The coding style used in the function in the linked code is pretty common. It uses nested if/else statements, and it reduces some repetition by only using the return keyword once and the radians call once.

That code could be improved using assignment operators to reduce more repetition, for example:

angle += 180

Perhaps this was one of your reasons to refactor the code, and that is understandable.

I like that your refactored function uses a consistent style: a simple condition followed by a return of a call to radians with a simple input. The repetitive use of return/radians slightly goes against DRY, as compared to the original code, but it is clean.

Ternary expression could be used, or even multiplication of 90 by boolean

I agree with you that these do not improve readability.

Documentation

I think both versions of the function (the original linked code and your chosen refactoring) could use more documentation, either as a docstring or as comments, explaining each condition.

\$\endgroup\$
3
  • 1
    \$\begingroup\$ "The repetitive use of return/radians slightly goes against DRY" I agree, but I though that making the function private and inside a wrapper only to DRY the radians was a little bit overkill. \$\endgroup\$ Commented Apr 23 at 12:56
  • \$\begingroup\$ About documentation, I agree, but I didn't touch the math that I wasn't able to fully comprehend. From what I know, opencv use 180based angle (not 360), but it is still very confusing to me, so this is something I'm not adressing/refactoring without more thorough investigation. \$\endgroup\$ Commented Apr 23 at 12:58
  • \$\begingroup\$ "the function in the linked code [...] reduces some repetition" IMO this is bad DRY, making it very confusing and hard to read. Yes, that's why I did the refactoring. \$\endgroup\$ Commented Apr 23 at 13:05
6
\$\begingroup\$

units

Teaching mypy about radians was a good idea, and your implementation is very nice. Feel free to keep it as-is.

Consider being more explicit about units at runtime, perhaps by adopting the Pint package. It's not the only one; unyt offers another good example.

The idea is to pass around an object that knows it represents e.g. angular measure, or mass. And then you can conveniently call methods which return radians or degrees, or which return grams or pounds, as desired. It's very explicit, both in the code and also in the default formatting of print() output.

div by zero

The width / height expression seems adventurous. I mean, did the contour finder and minAreaRect() truly guarantee there shall be no degenerate (zero pixel) rectangles that come back? If that's not in the documentation, then consider adding a tiny epsilon to the height.

math notation

This is lovely:

 near_square_ratio = 1.25 > (width / height) > 0.75

But please write it as a mathematician would, reflecting the convention of putting positive infinity on the right side of the page:

 near_square_ratio = 0.75 < (width / height) < 1.25

six vs. half-dozen

two ifs should be nested

benefit is too small

I agree. They're much the same, and what is in the OP code is already great. Just keep it as-is.

\$\endgroup\$
3
  • \$\begingroup\$ "math notation": My bad, I updated the order of comparison weeks ago, and completely forgot to updated it here ! I'm glad this is an opinion other have, I 100% agree on this \$\endgroup\$ Commented Apr 25 at 8:18
  • \$\begingroup\$ "div by zero" I don't know if it is, I agree with the espilon, thanks \$\endgroup\$ Commented Apr 25 at 8:21
  • \$\begingroup\$ "units" Yes, I know those libs, but don't use it yet. I wonder how it behaves with typechecker, builtins types and isinstance checks. I'll definitely look into that if typecheckers are not enough. \$\endgroup\$ Commented Apr 25 at 8:22

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.