Faster comparison of manifold points

[egourgoulhon]

Comparison of manifold points, as implemented in ManifoldPoint.__eq__, relies on comparison of coordinate values in a common chart. When coordinates are symbolic, this goes through Maxima and can take a lot of time. Now, comparison of points is involved in the unique representation of tangent spaces. When many tangent spaces are created (as for instance when plotting a vector field), this has a non-neglible cost. This ticket makes ManifoldPoint.__eq__ faster by invoking (a-b).is_trivial_zero() when a and b are symbolic coordinates of two points, while the previous version was using a == b. Note that for non-symbolic values (i.e. numerical values), a == b is still used.

[egourgoulhon]

New commits:

​34472e8

Faster comparison of manifold points

[roed]

Shouldn't you also short-circuit the for loop in case you ever get False?

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​bd717d9

Better comparison of manifold points

[egourgoulhon]

Replying to roed:

Shouldn't you also short-circuit the for loop in case you ever get False?

You are perfectly right! Thanks for pointing this. The latest commit takes it into account.

[roed]

The only other comment that I have is that you have three nested if statements, e.g.

if isinstance(diff_c, Expression):
    if not diff_c.is_trivial_zero():
        return False

I think it's clearer if such statements are phrased as

if isinstance(diff_c, Expression) and not diff_c.is_trivial_zero():
    return False

since there are fewer nested blocks this way.

Other than that, looks good once tests pass!

[egourgoulhon]

Replying to roed:

The only other comment that I have is that you have three nested if statements, e.g.

if isinstance(diff_c, Expression):
    if not diff_c.is_trivial_zero():
        return False

I think it's clearer if such statements are phrased as

if isinstance(diff_c, Expression) and not diff_c.is_trivial_zero():
    return False

since there are fewer nested blocks this way.

I wrote it with nested blocks because is_trivial_zero() is implemented only for instances of Expression. Is it safe to assume that the second form will never return any attribute error? i.e. that the test following the and is performed only if the test preceding the and is passed? (maybe this is guaranted by Python standard rules)

Other than that, looks good once tests pass!

The failures reported by the patchbot do not pertain to this ticket; on my computer all tests passed within Sage 8.1.beta1.

[roed]

Replying to egourgoulhon:

Replying to roed:

The only other comment that I have is that you have three nested if statements, e.g.

if isinstance(diff_c, Expression):
    if not diff_c.is_trivial_zero():
        return False

I think it's clearer if such statements are phrased as

if isinstance(diff_c, Expression) and not diff_c.is_trivial_zero():
    return False

since there are fewer nested blocks this way.

I wrote it with nested blocks because is_trivial_zero() is implemented only for instances of Expression. Is it safe to assume that the second form will never return any attribute error? i.e. that the test following the and is performed only if the test preceding the and is passed? (maybe this is guaranted by Python standard rules)

Yep, Python supports ​short-circuit evaluation for and and or.

[git]

Branch pushed to git repo; I updated commit sha1. New commits:

​48a66fc

Slight code simplification in manifold point comparison

[egourgoulhon]

Replying to roed:

Yep, Python supports ​short-circuit evaluation for and and or.

Very good, thanks. I've updated the code accordingly. I've also slightly simplified it, since there was no need to create a list of coordinate differences.

[roed]

The errors all have to do with docbuilding, which clearly isn't your faults since you don't edit any documentation.

Looks good to me.

[egourgoulhon]

Replying to roed:

The errors all have to do with docbuilding, which clearly isn't your faults since you don't edit any documentation.

Looks good to me.

Thanks for the review!