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:  sage8.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: 
Description (last modified by )
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
Branch:  → u/ghLoops7/25523_dyn_coeffs 

Cc:  Ben Hutz added 
Description:  modified (diff) 
Status:  new → needs_review 
comment:2 Changed 5 years ago by
Commit:  → ebb392f67cf4bb47d3ef8381f909b97c3f085583 

comment:3 Changed 5 years ago by
Status:  needs_review → needs_work 

comment:4 Changed 5 years ago by
Branch:  u/ghLoops7/25523_dyn_coeffs → u/ghLoops7/25523_ds_coeffs 

Commit:  ebb392f67cf4bb47d3ef8381f909b97c3f085583 
comment:5 Changed 5 years ago by
Commit:  → f7ad143dbf3ee1451b620dbf0761a08412b26a60 

Branch pushed to git repo; I updated commit sha1. New commits:
3c7aae1  25523: Add check in DynamicalSystems when a domain is specified that given polynomials have same base ring

246c570  25523:base_ring_check_ds

f7ad143  25523: Modified the check that polynomial shares base ring with given domain by adding coercion

comment:6 Changed 5 years ago by
Commit:  f7ad143dbf3ee1451b620dbf0761a08412b26a60 → 28d72ac0cb0187a369f990d3753b299393a2bf65 

Branch pushed to git repo; I updated commit sha1. New commits:
28d72ac  25523: Refactored base ring coercion to take into account checks already in method and removed errant apostrophe

comment:7 Changed 5 years ago by
Commit:  28d72ac0cb0187a369f990d3753b299393a2bf65 → 77bd02dec24d61df6b85d1efc8e62731bcc71b67 

Branch pushed to git repo; I updated commit sha1. New commits:
77bd02d  25523: reset undesirable change in minimal_model code that was necessary for the solution prerefactoring

comment:8 Changed 5 years ago by
Description:  modified (diff) 

Status:  needs_work → needs_review 
comment:9 Changed 5 years ago by
Commit:  77bd02dec24d61df6b85d1efc8e62731bcc71b67 → 02ae5ac355c665abf2dd7b386d6d0c904ef55680 

Branch pushed to git repo; I updated commit sha1. New commits:
02ae5ac  25523: Use coord_ring to get PolyRing

comment:10 Changed 5 years ago by
Reviewers:  → Ben Hutz 

Status:  needs_review → needs_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
Commit:  02ae5ac355c665abf2dd7b386d6d0c904ef55680 → 07d25b2676299042ab1ceb12f1620a0e7ef78ab0 

Branch pushed to git repo; I updated commit sha1. New commits:
07d25b2  25523: generalized coefficient coercion for ds

comment:12 Changed 5 years ago by
Status:  needs_work → needs_review 

comment:13 Changed 5 years ago by
Status:  needs_review → needs_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
Commit:  07d25b2676299042ab1ceb12f1620a0e7ef78ab0 → 9c85d312a251545daa7f5434e5336981663e674b 

Branch pushed to git repo; I updated commit sha1. New commits:
9c85d31  25523: Check dimension of space and # of polys match

comment:15 Changed 5 years ago by
Commit:  9c85d312a251545daa7f5434e5336981663e674b → bd22bded383bdf21ad500b8c8f6b8ff9637ed53a 

Branch pushed to git repo; I updated commit sha1. New commits:
bd22bde  25523: reworked domain construction

comment:16 Changed 5 years ago by
Commit:  bd22bded383bdf21ad500b8c8f6b8ff9637ed53a → fb6b726dd25a1ff5af806490b9e56dd0eb725dec 

Branch pushed to git repo; I updated commit sha1. New commits:
fb6b726  25523: remove unused imports

comment:17 Changed 5 years ago by
Status:  needs_work → needs_review 

comment:18 Changed 5 years ago by
Commit:  fb6b726dd25a1ff5af806490b9e56dd0eb725dec → 11b55055979c80cdbb772b1b4f22f5488d50ce8b 

Branch pushed to git repo; I updated commit sha1. New commits:
11b5505  25523: misplaced import

comment:19 Changed 5 years ago by
Branch:  u/ghLoops7/25523_ds_coeffs → u/bhutz/25523_ds_coeffs 

comment:20 Changed 5 years ago by
Commit:  11b55055979c80cdbb772b1b4f22f5488d50ce8b → 424a4f9ac2a9fcf216c4dd769655c404d66de5ab 

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:
424a4f9  25523: minor updates and tests added

comment:22 Changed 5 years ago by
Authors:  → Joseph Lupo 

Status:  needs_review → positive_review 
comment:23 Changed 4 years ago by
Branch:  u/bhutz/25523_ds_coeffs → 424a4f9ac2a9fcf216c4dd769655c404d66de5ab 

Resolution:  → fixed 
Status:  positive_review → closed 
Branch pushed to git repo; I updated commit sha1. New commits:
25513:check if endomorphism is already dynamical system in as_dynamical_system
25523: Add check in DynamicalSystems when a domain is specified that given polynomials have same base ring