#15920 closed enhancement (fixed)
Parallelize Possible Periods functions for Projective Morphisms
Reported by:  drose  Owned by:  drose 

Priority:  minor  Milestone:  sage6.2 
Component:  algebraic geometry  Keywords:  Projective, Morphism, Parallelization 
Cc:  bhutz  Merged in:  
Authors:  Dillon Rose  Reviewers:  Ben Hutz 
Report Upstream:  N/A  Work issues:  
Branch:  8bcc13a (Commits, GitHub, GitLab)  Commit:  
Dependencies:  #15966  Stopgaps: 
Description
Parallelize Possible Periods functions for Projective Morphisms
Change History (22)
comment:1 Changed 9 years ago by
Authors:  → Dillon Rose 

Branch:  → u/drose/15920 
Reviewers:  → Ben Hutz 
comment:2 Changed 9 years ago by
Branch:  u/drose/15920 → u/drose/ticket/15920 

comment:3 Changed 9 years ago by
Owner:  set to drose 

comment:4 Changed 9 years ago by
Commit:  → 6cf68fb06efe3160bf6cea766b26b2d176e9309d 

comment:5 Changed 9 years ago by
Cc:  bhutz added 

Keywords:  Projective Morphism Parallelization added 
comment:6 Changed 9 years ago by
Commit:  6cf68fb06efe3160bf6cea766b26b2d176e9309d → 500a0caa053ef4756e762e40bbd4f9455fba9b5d 

Branch pushed to git repo; I updated commit sha1. New commits:
500a0ca  Parallelized possible_periods function

comment:7 Changed 9 years ago by
Commit:  500a0caa053ef4756e762e40bbd4f9455fba9b5d → 8ab83fd966eb9e194e163b046f05400a439d712e 

Branch pushed to git repo; I updated commit sha1. New commits:
8ab83fd  Fixed paralleization of all_rational_preimages

comment:8 Changed 9 years ago by
Commit:  8ab83fd966eb9e194e163b046f05400a439d712e → 69dd970e65ad635573be4cc0cb0d8def1d442991 

Branch pushed to git repo; I updated commit sha1. New commits:
69dd970  Changed doctest to reflect changes

comment:9 Changed 9 years ago by
Dependencies:  → 15966 

Status:  new → needs_review 
These two changes looks fine. However, there is a third place that should be done in parallel.
periodic_points=self.lift_to_rational_periodic(pos_points,B)
in self.rational_periodic_points()
Also, it is convention not to use CamelCase variables names. (i.e. parallelData, possiblePeriods should be not have the capital in them).
I'm also marking 15966 as a dependency since without the cleanup of the child process these create too many child process to function well.
comment:10 Changed 9 years ago by
Status:  needs_review → needs_work 

comment:11 Changed 9 years ago by
Commit:  69dd970e65ad635573be4cc0cb0d8def1d442991 → 0fe665b4789cfb9fbc86f8c50df27607fa225da3 

comment:12 Changed 9 years ago by
Commit:  0fe665b4789cfb9fbc86f8c50df27607fa225da3 → ec4486365c82e3ab4266a73055904b0027af7652 

comment:13 Changed 9 years ago by
Dependencies:  15966 

comment:14 Changed 9 years ago by
Remember to set the status on tickets when it is ready for review again, but it is still needs_work so I'll leave it.
The functionality all checks out on my tests. However, with that last parallelization bit, there are 3 more doctests that are failing due to point order in projective_morphism.py. So you should sort those as well.
comment:15 Changed 9 years ago by
Commit:  ec4486365c82e3ab4266a73055904b0027af7652 → 8bcc13ad139b2b3f83ae3c2c105915310795b1b8 

Branch pushed to git repo; I updated commit sha1. New commits:
8bcc13a  Fixed bug in rational_periodic_points function.

comment:16 Changed 9 years ago by
Created:  Mar 12, 2014, 7:36:11 AM → Mar 12, 2014, 7:36:11 AM 

Dependencies:  → #15966 
Modified:  Apr 2, 2014, 9:44:34 PM → Apr 2, 2014, 9:44:34 PM 
comment:17 Changed 9 years ago by
Status:  needs_work → needs_review 

comment:18 Changed 9 years ago by
Status:  needs_review → needs_work 

This still has the 3 doctest failures due to sorting from
sage t long projective_morphism.py
comment:19 Changed 9 years ago by
Status:  needs_work → positive_review 

Actually, this is fine, I had failed to pull down all the commits.
comment:20 Changed 9 years ago by
Status:  positive_review → needs_work 

I'm getting some weird errors when I run the possible_periods over and over again (even on the same function). It seems to be somewhat random which error and whether or not an error occurs). I may or may not have corrupted my sage, but I don't want to leave this positive_review until I resolve it.
Errors like
Traceback (most recent call last): ... TypeError: number of arguments does not match number of variables in parent
and
Traceback (most recent call last): .... TypeError: unsupported operand parent(s) for '*': 'Finite Field of size 2' and 'Finite Field of size 13'
comment:21 Changed 9 years ago by
Branch:  u/drose/ticket/15920 → 8bcc13ad139b2b3f83ae3c2c105915310795b1b8 

Resolution:  → fixed 
Status:  needs_work → closed 
comment:22 Changed 9 years ago by
Commit:  8bcc13ad139b2b3f83ae3c2c105915310795b1b8 

I had it already tested and it worked on the buildbot. Reopen if you can reproduce your failure...
Branch pushed to git repo; I updated commit sha1. New commits:
Parallelized all_rational_preimages function