Opened 5 years ago

Closed 4 years ago

#25523 closed defect (fixed)

Raise Exception if DynamicalSystem initialized with coeffs not in given domain

Reported by: Joey Lupo Owned by:
Priority: minor Milestone: sage-8.3
Component: dynamics Keywords:
Cc: Ben Hutz Merged in:
Authors: Joseph Lupo Reviewers: Ben Hutz
Report Upstream: N/A Work issues:
Branch: 424a4f9 (Commits, GitHub, GitLab) Commit: 424a4f9ac2a9fcf216c4dd769655c404d66de5ab
Dependencies: Stopgaps:

Status badges

Description (last modified by Joey Lupo)

Currently, code such as

P.<x,y>=ProjectiveSpace(QQ,1) 
f=DynamicalSystem([CC.0*x^2+2*y^2,1*y^2], domain=P)

does not raise an error as it should. We address this by raising a TypeError? if the given polynomials have a different base ring than the domain.

Change History (23)

comment:1 Changed 5 years ago by Joey Lupo

Branch: u/gh-Loops7/25523_dyn_coeffs
Cc: Ben Hutz added
Description: modified (diff)
Status: newneeds_review

comment:2 Changed 5 years ago by git

Commit: ebb392f67cf4bb47d3ef8381f909b97c3f085583

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

0834abb25513:check if endomorphism is already dynamical system in as_dynamical_system
ebb392f25523: Add check in DynamicalSystems when a domain is specified that given polynomials have same base ring

comment:3 Changed 5 years ago by Joey Lupo

Status: needs_reviewneeds_work

comment:4 Changed 5 years ago by Joey Lupo

Branch: u/gh-Loops7/25523_dyn_coeffsu/gh-Loops7/25523_ds_coeffs
Commit: ebb392f67cf4bb47d3ef8381f909b97c3f085583

comment:5 Changed 5 years ago by git

Commit: f7ad143dbf3ee1451b620dbf0761a08412b26a60

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

3c7aae125523: Add check in DynamicalSystems when a domain is specified that given polynomials have same base ring
246c57025523:base_ring_check_ds
f7ad14325523: Modified the check that polynomial shares base ring with given domain by adding coercion

comment:6 Changed 5 years ago by git

Commit: f7ad143dbf3ee1451b620dbf0761a08412b26a6028d72ac0cb0187a369f990d3753b299393a2bf65

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

28d72ac25523: Refactored base ring coercion to take into account checks already in method and removed errant apostrophe

comment:7 Changed 5 years ago by git

Commit: 28d72ac0cb0187a369f990d3753b299393a2bf6577bd02dec24d61df6b85d1efc8e62731bcc71b67

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

77bd02d25523: reset undesirable change in minimal_model code that was necessary for the solution pre-refactoring

comment:8 Changed 5 years ago by Joey Lupo

Description: modified (diff)
Status: needs_workneeds_review

comment:9 Changed 5 years ago by git

Commit: 77bd02dec24d61df6b85d1efc8e62731bcc71b6702ae5ac355c665abf2dd7b386d6d0c904ef55680

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

02ae5ac25523: Use coord_ring to get PolyRing

comment:10 Changed 5 years ago by Ben Hutz

Reviewers: Ben Hutz
Status: needs_reviewneeds_work

A couple minor things first

  • extra blank lines added in affine_ds are not needed
  • is_Quotient import is not used

Now for some still weird functionality

  • The following gets a typeerror
    P.<x,y>=ProjectiveSpace(ZZ,1)
    f=DynamicalSystem([3*x^2, 1/5*y^2], domain=P)
    

but this gives back a dynamical system over integer ring

P.<x,y>=ProjectiveSpace(ZZ,1)
f=DynamicalSystem([3*x^2, 1/5*y^2])
  • when you pass in an endomorphism the domain isn't used. This may be ok if it is documented this way or maybe we should try to coerce to domain.
    P.<x,y>=ProjectiveSpace(QQ,1)
    P2.<x,y>=ProjectiveSpace(CC,1)
    H = End(P2)
    f=H([CC.0*x^2,y^2])
    g=DynamicalSystem(f, domain=P)
    
  • When you don't give it a domain, you can get some very weird behavior
    P.<x,y>=ProjectiveSpace(ZZ,1)
    f=DynamicalSystem([GF(5)(3)*x^2, CC.0*y^2])
    

notice the two coordinate polynomials don't even have the same parent!

f[0].parent(),f[1].parent()

comment:11 Changed 5 years ago by git

Commit: 02ae5ac355c665abf2dd7b386d6d0c904ef5568007d25b2676299042ab1ceb12f1620a0e7ef78ab0

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

07d25b225523: generalized coefficient coercion for ds

comment:12 Changed 5 years ago by Joey Lupo

Status: needs_workneeds_review

comment:13 Changed 5 years ago by Ben Hutz

Status: needs_reviewneeds_work

A couple issues here.

In projective.ds

R.<x>=QQ[]
DynamicalSystem_projective([x, x+1])

This is a weird error. I think you should do a little sanity checking before creating the domain.

In this one, it is functioning correctly, but I think you should add what domain you are attempting to use to the error message since it is not passed in by the user

R.<x,y,z>=QQ[]
f=DynamicalSystem_projective([x+y+z,y*z])

you need to add a couple doc tests showing the behavior you are correcting, especially an example, where the domain is created based off of the parent of the first coordinate function


in affine.ds

definitely missing some checks somewhere in here

R.<x>=QQ[]
DynamicalSystem_affine([x, x+1,2*x])

and

R.<x,y,z>=QQ[]
f=DynamicalSystem_affine([x+y+z,y*z])

notice the dimension of the affine space these are creating as a domain. Need to sanity check the number of polys matches the dimension

affine also needs some doc tests showing the behavior.

comment:14 Changed 5 years ago by git

Commit: 07d25b2676299042ab1ceb12f1620a0e7ef78ab09c85d312a251545daa7f5434e5336981663e674b

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

9c85d3125523: Check dimension of space and # of polys match

comment:15 Changed 5 years ago by git

Commit: 9c85d312a251545daa7f5434e5336981663e674bbd22bded383bdf21ad500b8c8f6b8ff9637ed53a

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

bd22bde25523: reworked domain construction

comment:16 Changed 5 years ago by git

Commit: bd22bded383bdf21ad500b8c8f6b8ff9637ed53afb6b726dd25a1ff5af806490b9e56dd0eb725dec

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

fb6b72625523: remove unused imports

comment:17 Changed 5 years ago by Joey Lupo

Status: needs_workneeds_review

comment:18 Changed 5 years ago by git

Commit: fb6b726dd25a1ff5af806490b9e56dd0eb725dec11b55055979c80cdbb772b1b4f22f5488d50ce8b

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

11b550525523: misplaced import

comment:19 Changed 5 years ago by Ben Hutz

Branch: u/gh-Loops7/25523_ds_coeffsu/bhutz/25523_ds_coeffs

comment:20 Changed 5 years ago by Ben Hutz

Commit: 11b55055979c80cdbb772b1b4f22f5488d50ce8b424a4f9ac2a9fcf216c4dd769655c404d66de5ab

everything seems to be working well here. I made some minor doc updates, added a couple examples, and made one code change. When the domain is not given, you already have the common parent, so it was just a minor optimization.

If you're fine with these changes, we'll go ahead and mark it positive.


New commits:

424a4f925523: minor updates and tests added

comment:21 Changed 5 years ago by Joey Lupo

This all looks good to me- good to go.

comment:22 Changed 5 years ago by Ben Hutz

Authors: Joseph Lupo
Status: needs_reviewpositive_review

comment:23 Changed 4 years ago by Volker Braun

Branch: u/bhutz/25523_ds_coeffs424a4f9ac2a9fcf216c4dd769655c404d66de5ab
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.