3

I'm currently writing a program in Python to model projective geometry, and the congruence function for a projective point is looking rather nasty.

(for anyone interested, two projective points are congruent if they both lie on a single line passing through the origin.)

class Point(object):
    def __init__(self, a, b, c):
        self.coords = [ a, b, c ]

    def congruent(self, other):
        ratio = 0
        for i in range(3):
            if self.coords[i] != 0 and other.coords[i] != 0:
                if ratio is 0:
                    ratio = other.coords[i] / self.coords[i]
                elif ratio != other.coords[i] / self.coords[i]:
                    return False
            elif self.coords[i] != 0 or other.coords[i] != 0:
                return False
        return True

I'm new to Python, but I know that there's generally a "Pythonic" way to do everything. With that in mind, how would I go about making this more readable?

tzaman
  • 46,925
  • 11
  • 90
  • 115
Patrick
  • 455
  • 4
  • 11
  • Question about your code: you set `ratio = 0` but then check `if ratio is 0:`, is something missing? Why would `ratio` be anything but 0? – Scott May 04 '15 at 19:40
  • 1
    This question seems to be subjective, discussion and opinion based... Doesn't really have an objective answer. – Yeo May 04 '15 at 19:44
  • 1
    After the code checks ratio is 0, it assigns ratio to a new value. After that happens, ratio should never be reassigned. There's probably a better way to do that, though. – Patrick May 04 '15 at 19:46
  • 1
    @Patrick, Set ratio using the 0 coords outside the loop, then start your loop at index 1 instead. That will make it clearer that it isn't expected to change during the loop. – Paul Butcher May 04 '15 at 19:53
  • @PaulButcher that's what I was getting at. – Scott May 04 '15 at 19:54
  • 1
    I'm voting to close this question as off-topic because it should be migrated to http://codereview.stackexchange.com/ – Robert H May 04 '15 at 20:00
  • 5
    @RobertH Closing questions help nothing for migration, if you feel that it should be migrated, click 'flag' -> in need of moderator intervention -> say that you think it should be migrated. – Simon Forsberg May 04 '15 at 20:02
  • @PaulButcher I was worried about division by zero, since the 0 coords aren't guaranteed to both be nonzero. – Patrick May 04 '15 at 20:03
  • @Patrick Ok now I understand why you had that out there. – Scott May 04 '15 at 20:07
  • 1
    @SimonAndréForsberg not sure about flagging for mod intervention; they generally don't have time for this. See this [meta question](http://meta.stackexchange.com/questions/228626/for-questions-which-might-belong-on-maths-stats-etc-is-is-better-to-vote-to-c). – TooTone May 04 '15 at 20:10
  • @RobertH yeah I guess it would have been better at CodeReview. My bad. – Patrick May 04 '15 at 20:16
  • 1
    @TooTone It surely can take a while for the flags to be handled, but I have good experience with flagging for migration. Although I have a high amount of reputation on CR, which makes moderators trust me more. In addition, RobertH's close reason did not specifically say that it is ***off-topic for Stack Overflow***, which makes it a horrible close-reason overall. "I'm voting to close this question as off-topic because it belongs at (other site)" is not a valid close reason. If you want to discuss more, you can find me in [2nd monitor](http://chat.stackexchange.com/rooms/8595/the-2nd-monitor) – Simon Forsberg May 04 '15 at 20:18
  • 1
    @RobertH Being on-topic on another Stack Exchange site does not, by itself, make a question off-topic where it is posted. See [this](http://meta.stackexchange.com/questions/228626/for-questions-which-might-belong-on-maths-stats-etc-is-is-better-to-vote-to-c) and [this](http://meta.stackoverflow.com/questions/287400/does-being-on-topic-at-another-stack-exchange-site-automatically-make-a-question). This question *may* be on-topic for [Code Review](http://codereview.stackexchange.com/help), but the first criteria for question migration is that the question be off-topic on the source site. – nhgrif May 04 '15 at 20:28
  • @TooTone You can retract your close vote (though if you feel this question is off-topic for Stack Overflow, you shouldn't), but I'd recommend leaving a comment explaining to the asker why this question doesn't belong on Stack Overflow. – nhgrif May 04 '15 at 20:29
  • @SimonAndréForsberg et al: Closing was my opinion as I felt this style of question was better migrated - If you disagree, that's fine. May a flag been more appropriate? - Perhaps. Alas, I chose to cast a close vote and while I'm glad to have sparked discussion, I am sad that its detracted away from the question itself, which I do find a valuable contribution to the community, regardless of which site it rests on. – Robert H May 04 '15 at 23:35

3 Answers3

3

How about this:

def congruent(self, other, eps=0.001):
    ratios = (c1 / c2 for c1, c2 in zip(self.coords, other.coords) if c1 or c2)
    try:
        first = next(ratios)
        return all(abs(ratio - first) < eps for ratio in ratios)
    except ZeroDivisionError:
        return False
  1. Prefer operating directly over elements instead of on indices if possible (zip is handy).
  2. The list comprehension gets all coord ratios for cases where either coordinate is non-zero. If both are, it's fine, and it gets excluded.
  3. The ZDE only happens if c1 is nonzero and c2 is zero, so that's a fail.
  4. At the end, we pass if all ratios are equal.

Note: If you're not using Python 3, you should add from __future__ import division to the top of your file so that you don't get incorrect results for integer coordinate values.

Edit: added short-circuiting and epsilon-comparison for float ratios per @JoranBeasley.

tzaman
  • 46,925
  • 11
  • 90
  • 115
  • bah better than mine even. ... I feel like you could use some short circuiting with all and a generator ... but great answer +1 – Joran Beasley May 04 '15 at 20:01
  • @JoranBeasley Yeah I thought about putting the `all` into the `try`, it's a simple substitution but it harms readability a bit. – tzaman May 04 '15 at 20:02
  • `0.0/0.0` does give me a ZDE so not entirely sure that bit will work as expected but looks believe-able enough – Joran Beasley May 04 '15 at 20:04
  • 2
    @JoranBeasley added short-circuit, check it out :). If both are `0.0` then they'll get excluded from the ratio list since `if c1 or c2` is `False`. – tzaman May 04 '15 at 20:05
  • `Point(1,1,1).congruent(Point(0,0,0))` returns False – Stefan Pochmann May 04 '15 at 21:13
1

Maybe use if self.coords[i] instead of if self.coords[i] != 0 (same for similar examples) and if not ratio instead of if ratio is 0. In Python, any nonzero value passes if clause, so you don't need to check if it is nonzero, it is automatic.

Adalee
  • 528
  • 12
  • 26
1
def congurent(self,other):
    ratio = None
    for a,b in zip(self,other):
        if a != 0 and b != 0:
            if ratio is None: 
                 ratio = a/float(b)
            elif abs(ratio - a/float(b))>0.001:
                 return False
        elif a!=0 or b!=0:
            return False
     return True

is perhaps a little more pythonic... although all its really changing is how you iterate over the list (its the same number of lines)

Joran Beasley
  • 110,522
  • 12
  • 160
  • 179