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

Refactor IntegerMulAction #24467

Closed
jdemeyer opened this issue Jan 3, 2018 · 15 comments
Closed

Refactor IntegerMulAction #24467

jdemeyer opened this issue Jan 3, 2018 · 15 comments

Comments

@jdemeyer
Copy link

jdemeyer commented Jan 3, 2018

Refactor IntegerMulAction in preparation for #24247:

  1. Add an abstract base class IntegerAction which will also be used to implement IntegerPowAction in Implement __pow__ in the coercion model #24247.

  2. Add a new helper function parent_is_integers to check if some type/parent represents the integers.

  3. Instead of the hacks in discover_action and verify_action involving an explicit IntegerMulAction check, return an IntegerMulAction for anything satisfying parent_is_integers().

Component: coercion

Author: Jeroen Demeyer

Branch/Commit: a359862

Reviewer: Travis Scrimshaw

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

@jdemeyer jdemeyer added this to the sage-8.2 milestone Jan 3, 2018
@jdemeyer
Copy link
Author

jdemeyer commented Jan 3, 2018

Branch: u/jdemeyer/ticket/24467

@jdemeyer
Copy link
Author

jdemeyer commented Jan 3, 2018

Commit: 85cabe7

@jdemeyer
Copy link
Author

jdemeyer commented Jan 3, 2018

New commits:

85cabe7Refactor IntegerMulAction

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 4, 2018

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

a2d60efAlso verify IntegerAction instances

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 4, 2018

Changed commit from 85cabe7 to a2d60ef

@jdemeyer

This comment has been minimized.

@tscrim
Copy link
Collaborator

tscrim commented Jan 8, 2018

Reviewer: Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Jan 8, 2018

comment:5

The · is not ascii nor latex: n · a = a + a + ... + a. So if you change that to \cdot (personally, I would also change the ... to \cdots), then you can set a positive review on my behalf.

@jdemeyer
Copy link
Author

jdemeyer commented Jan 8, 2018

comment:6

Replying to @tscrim:

The · is not ascii nor latex

Is that a problem really? The documentation builds fine and I think that · is more readable than \cdot in text mode.

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 8, 2018

Changed commit from a2d60ef to a359862

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Jan 8, 2018

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

a359862Use \cdot instead of ·

@tscrim
Copy link
Collaborator

tscrim commented Jan 8, 2018

comment:9

Replying to @jdemeyer:

Replying to @tscrim:

The · is not ascii nor latex

Is that a problem really? The documentation builds fine and I think that · is more readable than \cdot in text mode.

With the non-ascii characters, the pdf docs tend to have difficulty if the encoding is not declared (in particular, the non-ascii hyphen comes to mind). What we probably should do is add a bunch of the unicode versions of certain latex commands to the console doc replacer since we generally assume terminals support unicode.

Thanks for the change.

@jdemeyer
Copy link
Author

jdemeyer commented Jan 8, 2018

comment:10

Replying to @tscrim:

What we probably should do is add a bunch of the unicode versions of certain latex commands to the console doc replacer since we generally assume terminals support unicode.

No, we do the opposite: replace certain Unicode characters by latex when building the PDF docs. In src/doc/common/conf.py you will find for example

\DeclareUnicodeCharacter{00B7}{\cdot}

saying that · should be replaced by \cdot. So · in docstrings should work just fine.

@tscrim
Copy link
Collaborator

tscrim commented Jan 8, 2018

comment:11

I see. I wasn't aware of that. Thank you.

@vbraun
Copy link
Member

vbraun commented Jan 14, 2018

Changed branch from u/jdemeyer/ticket/24467 to a359862

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