Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize ManinSymbol #18388

Closed
jdemeyer opened this issue May 9, 2015 · 10 comments
Closed

Optimize ManinSymbol #18388

jdemeyer opened this issue May 9, 2015 · 10 comments

Comments

@jdemeyer
Copy link

jdemeyer commented May 9, 2015

Instead of storing the tuple (i,u,v) as a tuple, it would be a lot better and simpler to store it as 3 different attributes.

We also fix comparison to use _cmp_ (which is required for extension types) instead of __cmp__.

Component: cython

Author: Jeroen Demeyer

Branch/Commit: d70023e

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/18388

@jdemeyer jdemeyer added this to the sage-6.7 milestone May 9, 2015
@jdemeyer

This comment has been minimized.

@jdemeyer jdemeyer changed the title Fix comparison for ManinSymbol Optimize ManinSymbol May 9, 2015
@jdemeyer

This comment has been minimized.

@jdemeyer
Copy link
Author

jdemeyer commented May 9, 2015

@jdemeyer
Copy link
Author

jdemeyer commented May 9, 2015

Commit: 07ad588

@jdemeyer
Copy link
Author

jdemeyer commented May 9, 2015

New commits:

07ad588Optimize ManinSymbol class

@tscrim
Copy link
Collaborator

tscrim commented May 9, 2015

comment:5

Very minor point, but you could do the alignment:

-       return self.__class__(self.parent(),
-                             (self.i,
-                              matrix[0]*self.u + matrix[2]*self.v,
-                              matrix[1]*self.u + matrix[3]*self.v))
+       return type(self)(self.parent(),
+                         (self.i,
+                          matrix[0]*self.u + matrix[2]*self.v,
+                          matrix[1]*self.u + matrix[3]*self.v))

After that, you can set a positive review on my behalf.

@tscrim
Copy link
Collaborator

tscrim commented May 9, 2015

Reviewer: Travis Scrimshaw

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 10, 2015

Changed commit from 07ad588 to d70023e

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented May 10, 2015

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

d70023eFix indentation (PEP 8)

@vbraun
Copy link
Member

vbraun commented May 13, 2015

Changed branch from u/jdemeyer/fix_comparison_for_maninsymbol to d70023e

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

No branches or pull requests

3 participants