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

Issues with ordinals module #14752

Open
asmeurer opened this issue May 29, 2018 · 4 comments
Open

Issues with ordinals module #14752

asmeurer opened this issue May 29, 2018 · 4 comments
Labels

Comments

@asmeurer
Copy link
Member

I just noticed the new ordinals module (#13682). There are a few issues with it (sorry I didn't notice this when it was being reviewed).

  • The printing should be defined in the printers in sympy.printing, not in __str__ (also side note here, methods should not define behavior for subclasses. If you want to define behavior on a subclass, put the method on the subclass. I'm referring to this line).
  • Why is the class Basic? The operations produce Mul, so either the class should be Expr, or the ordinal operators should use their own classes.
  • The classes should not be on S (I am removing them over at Add safe flag to sympify() #12524 so don't worry about this one).
  • The str printer should print omega, since that is what the variable name is called.
  • The pretty printer should use ω instead of w.
  • Doing fancy stuff in __eq__ is generally considered an antipattern in SymPy (objects should compare with == structurally, not mathematically). I haven't dug into what it is actually doing, though, so this may be OK.

CC @ashishkg0022

@ashishkg0022
Copy link
Contributor

We discussed a few points while implementing:

  • class can't be Expr , because the addition is non - commutative.

  • work was to be extended later by adding a pretty printer and a latex printer.

@asmeurer
Copy link
Member Author

I see. Actually, for some reason when I was testing this before multiplying ordinals gave Mul, but now when I test it they only give Ordinal. I'm not sure what happened there.

@oscarbenjamin
Copy link
Contributor

What's the status of this?

The comments above indicate that #13682 would be followed up with changes to the printers but that was 8 months ago. Did that happen?

@asmeurer
Copy link
Member Author

__str__ is still defined on Ordinal so I guess not.

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

No branches or pull requests

3 participants