Opened 5 years ago
Closed 5 years ago
#22746 closed enhancement (fixed)
Update sage.lfunctions.dokchitser.Dokchitser to use only one gp interpreter
Reported by: | embray | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-8.0 |
Component: | elliptic curves | Keywords: | |
Cc: | Merged in: | ||
Authors: | Erik Bray | Reviewers: | Jean-Pierre Flori |
Report Upstream: | N/A | Work issues: | |
Branch: | 2cc45fa (Commits, GitHub, GitLab) | Commit: | 2cc45fa219cfbe5a55ba3722ae329e706f2ca0ee |
Dependencies: | Stopgaps: |
Description
This updates the Dokchitser
class to share a single gp
interpreter instance for all instances of the Dokchitser
class. Previously each instance spun up its own gp
interpreter, and would keep it running until the instance is garbage-collected.
That situation was less than ideal in general, and even worse on platforms where file descriptors are limited (which could be any depending on the user's ulimits). See #22718
This provides a temporary workaround to something like #15809, which might be a preferable solution. But this solution was quick to hack together, works now, is needed for Cygwin, and will be easy enough to excise once a better solution is available. Please read the commit message for more implementation details.
(please set the correct component if not "elliptic curves"--it just seemed the most relevant out of what was available)
Change History (11)
comment:1 Changed 5 years ago by
comment:2 Changed 5 years ago by
- Status changed from new to needs_review
comment:3 Changed 5 years ago by
- Commit changed from 0af0f4c3b78f8fffa69c644e8da1c427e2da8b17 to 2573de5b00d1839ea85cc3ac2bfc5b59c2f843e8
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
2573de5 | Modify the Dokchitser class to only require one gp interpreter which can
|
comment:4 Changed 5 years ago by
- Commit changed from 2573de5b00d1839ea85cc3ac2bfc5b59c2f843e8 to 44fe7702457325c6ec5b6bebbbce0e3a68ec3be0
Branch pushed to git repo; I updated commit sha1. New commits:
10fc86d | These variables aren't actually globals in this case and don't need to be indexed.
|
44fe770 | Unforunate (but working) hack needed in case any of the instantiation 'pre-code' references any of the indexed globals in the script template.
|
comment:5 Changed 5 years ago by
- Commit changed from 44fe7702457325c6ec5b6bebbbce0e3a68ec3be0 to 063041cb44f3ee42626ce6848b705b58a401f497
Branch pushed to git repo; I updated commit sha1. New commits:
063041c | Use the appropriate built-in method for this
|
comment:6 Changed 5 years ago by
- Status changed from needs_review to needs_work
Still finding new issues with this...
comment:7 Changed 5 years ago by
- Commit changed from 063041cb44f3ee42626ce6848b705b58a401f497 to 492f57b5db58a005b7ef5c684edcf90449297be1
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
906cc66 | No need to unset CFLAGS for numpy
|
2763fc9 | Deprecation warning message should guide a user to import lcm from sage.arith.all
|
ee626ea | 22556: implement periodic points for rational maps
|
cff3c89 | 22556: fixed doc typo
|
308d4f2 | 22556: fix indeterminacy_locus
|
4471c21 | Updated SageMath version to 8.0.beta2
|
a2a05ad | Modify the Dokchitser class to only require one gp interpreter which can
|
17b67d4 | Unforunate (but working) hack needed in case any of the instantiation 'pre-code' references any of the indexed globals in the script template.
|
7e949ba | Use the appropriate built-in method for this
|
492f57b | Initializing this attribute should be moved to the end of init_coeffs so that errors in initLdata are still raised as exceptions.
|
comment:8 Changed 5 years ago by
- Commit changed from 492f57b5db58a005b7ef5c684edcf90449297be1 to 2cc45fa219cfbe5a55ba3722ae329e706f2ca0ee
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
dd6c49c | Modify the Dokchitser class to only require one gp interpreter which can
|
a16bb52 | Unforunate (but working) hack needed in case any of the instantiation 'pre-code' references any of the indexed globals in the script template.
|
1aad963 | Use the appropriate built-in method for this
|
2cc45fa | Initializing this attribute should be moved to the end of init_coeffs so that errors in initLdata are still raised as exceptions.
|
comment:9 Changed 5 years ago by
- Status changed from needs_work to needs_review
Okay, looks like I have this right now. I actually did have it working before but broke it with an unnecessary earlier commit that stemmed from misunderstanding how variable scoping works in pari.
comment:10 Changed 5 years ago by
- Reviewers set to Jean-Pierre Flori
- Status changed from needs_review to positive_review
comment:11 Changed 5 years ago by
- Branch changed from u/embray/lfunctions/dokchitser to 2cc45fa219cfbe5a55ba3722ae329e706f2ca0ee
- Resolution set to fixed
- Status changed from positive_review to closed
A possible enhancement would be to allow an N-process pool, for faster parallel computation. But I don't know if that's really an important use-case here.