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

Should be there both PolyRing and PolynomialRing? #14220

Open
skirpichev opened this Issue Feb 15, 2018 · 7 comments

Comments

Projects
None yet
2 participants
@skirpichev
Copy link
Contributor

skirpichev commented Feb 15, 2018

It seems that they represent multivariate polynomial rings (elements of which are PolyElement's). Is it intentional or some temporary state? Probably, there should be one class for domain (i.e. PolynomialRing) and one class for elements.

@jksuom

This comment has been minimized.

Copy link
Member

jksuom commented Feb 16, 2018

I am guessing, as the documentation seems incomplete, that PolynomialRing and PolyRing are planned as two levels of the implementation, a high level class with stable public interface and a low level class for internal use. This need not necessarily be always so. The polynomial module has been rewritten several times. (The "second complete rewrite" appeared already in 2009.)

@skirpichev

This comment has been minimized.

Copy link
Contributor

skirpichev commented Feb 16, 2018

PolynomialRing and PolyRing are planned

I'm not sure that there was something "planned" (taking into account also several rewrites).

Some digging into the history:

  1. PolynomialRing was added: e4b53b3 (and uses DMP as dtype)
  2. PolyRing added: 90a9eff, #1840
  3. PolynomialRingNG added (new sparse polys): 01e0a8f
  4. Added class factory in PolyRing (later created types will be renamed to PolyElement for more fun!): d9a2079

Nothing makes clear why there should be additional class, besides PolynomialRing. My guess, that this class was a some remnant of porting lpoly2 stuff (see #609).

a high level class with stable public interface and a low level class for internal use

OK, and what's a purpose of the private class PolyRing?

(As it's expected, after main author of the module leaving "welcoming and friendly" community, nobody actually knows what implemented here and why.)

(The "second complete rewrite" appeared already in 2009.)

Btw, it seems this was submitted not via pull request and I don't see where this stuff was discussed in google groups sympy/sympy-paches.

@skirpichev

This comment has been minimized.

Copy link
Contributor

skirpichev commented Mar 5, 2018

BTW, same issue for FracField vs FractionField. Perhaps, this might be easier to start with.

skirpichev added a commit to skirpichev/diofant that referenced this issue Mar 5, 2018

skirpichev added a commit to skirpichev/diofant that referenced this issue Mar 5, 2018

skirpichev added a commit to skirpichev/diofant that referenced this issue Mar 5, 2018

skirpichev added a commit to skirpichev/diofant that referenced this issue Mar 6, 2018

skirpichev added a commit to skirpichev/diofant that referenced this issue Mar 6, 2018

skirpichev added a commit to skirpichev/diofant that referenced this issue Mar 7, 2018

skirpichev added a commit to skirpichev/diofant that referenced this issue Mar 7, 2018

polys: remove PolyRing, make PolynomialRing domain
Also make get_ring() method for Ring's - a property "ring"

Closes sympy/sympy#14220

skirpichev added a commit to skirpichev/diofant that referenced this issue Mar 7, 2018

polys: remove PolyRing, make PolynomialRing domain
Also make get_ring() method for Ring's - a property "ring"

Closes sympy/sympy#14220

skirpichev added a commit to skirpichev/diofant that referenced this issue Mar 8, 2018

polys: remove PolyRing, make PolynomialRing domain
Also make get_ring() method for Ring's - a property "ring"

Closes sympy/sympy#14220
@skirpichev

This comment has been minimized.

Copy link
Contributor

skirpichev commented Mar 8, 2018

It turns that removing of FracField/PolyRing wasn't too hard.

One side issue: order property of PolynomialRing/FractionField's. Does it make sense to have it? As an alternative, we could provide order kwarg (with default value) for methods that assume some total order in given domain.

@jksuom

This comment has been minimized.

Copy link
Member

jksuom commented Mar 8, 2018

It seems to me that it would not be necessary to have the order attribute. In fact, it might even be more convenient not to keep it as a part of the ring/field object. If it is omitted, then some Gröbner methods would have to be edited, but that would probably not be too hard.

@skirpichev

This comment has been minimized.

Copy link
Contributor

skirpichev commented Mar 9, 2018

some Gröbner methods would have to be edited, but that would probably not be too hard.

I'm not sure. E.g. LM properties will be affected by this change and there is a lot of code in Gröbner module which access these properties.

@jksuom

This comment has been minimized.

Copy link
Member

jksuom commented Mar 9, 2018

LM, etc. may have been the reason why order was included as an attribute. If that is omitted, those methods would have to be rewritten to accept order as an argument.

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