Opened 6 years ago
Closed 5 years ago
#18555 closed enhancement (fixed)
Pickling and otherwise enhancing global options
Reported by: | andrew.mathas | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-7.3 |
Component: | interfaces | Keywords: | days78, options |
Cc: | ncohen, tscrim | Merged in: | |
Authors: | Andrew Mathas | Reviewers: | Travis Scrimshaw |
Report Upstream: | N/A | Work issues: | |
Branch: | ba7dd98 (Commits) | Commit: | ba7dd9836386ed119c257d9bb42030149e091b47 |
Dependencies: | Stopgaps: |
Description (last modified by )
Instances of the GlobalOptions class in sage.structure.global_options
do not pickle, which is annoying and causes various problems. (This is a defect.)
In addition, when the GlobalOptions
class was introduced it was suggested in sage-dev that it "would be nice" if we could implement the syntax used in the IPython configuration:
A.options.foobar = 1 in addition to A.options(foobar=1)
This ticket implements both of these features and, in addition, makes it possible to construct options classes dynamically.
As was also suggested on sage-dev, I have also renamed all of the global_options methods simply as options, with the global_options variants being deprecated. The "stand-alone" options classes such as PartitionOoptions
, TableauxOptions
, ... have also been deprecated and, instead, put inside their "parent" classes where they are accessible using the options
method (global_options
methods have been deprecated). This is cleaned up the code a little and it seems to speed up the sage start-up time.
The pickling is done by adding an extra module
argument to the GlobalOptions
that specifies the module which contains the class that the options are attached -- name
of of the class defaults to the name of the options class but this can be explicitly set using the options_class
argument. The options class is assumed to have an options
method, and this is used to unpickle a pickle for an options class. GlobalOptions
that do not arise as options methods for standard sage classes cannot be unpickled. The reason why the name of the module and class are passed, as strings, to GlobalOptions
rather than the actual class is that this approach allows the options to be constructed during the initialisation of the (instance of the) class and, in turn, this allows the class to dynamically construct their options classes.
Change History (30)
comment:1 Changed 6 years ago by
- Cc ncohen added
comment:2 Changed 6 years ago by
- Component changed from PLEASE CHANGE to interfaces
- Description modified (diff)
- Keywords options added
- Summary changed from Pickling global options to Pickling and otherwise enhancing global options
- Type changed from PLEASE CHANGE to enhancement
comment:3 Changed 6 years ago by
comment:4 Changed 6 years ago by
- Description modified (diff)
comment:5 Changed 6 years ago by
- Cc tscrim added
comment:6 Changed 6 years ago by
- Branch set to u/andrew.mathas/pickling_global_options
comment:7 Changed 6 years ago by
- Commit set to 2ba392f42e9ee4b362cf61e8ddd2a9d419f0787e
- Description modified (diff)
comment:8 Changed 6 years ago by
- Description modified (diff)
comment:9 Changed 5 years ago by
- Commit changed from 2ba392f42e9ee4b362cf61e8ddd2a9d419f0787e to 12cb84666d10cab49c8d8b21aad73485c2fedaef
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
3a4a9d2 | 20618: imported sage.modules.tutorial_free_modules and thematic_tutorials/tutorial-implementing-algebraic-structures from the Sage-Combinat queue
|
3e3e350 | 20618: fixed missing file
|
500d2d8 | Global options to options
|
52d7cf6 | Recasting options as attributes
|
64d0209 | Global options as class attributes
|
25ab72f | Fixing doctests
|
23f2c56 | Merging into 7.3.beta4
|
12cb846 | Fixing more doc-tests
|
comment:10 Changed 5 years ago by
- Description modified (diff)
comment:11 follow-up: ↓ 12 Changed 5 years ago by
- Commit changed from 12cb84666d10cab49c8d8b21aad73485c2fedaef to 11b7080fb16f62f64d39bca2286d0912c55a2125
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
c676864 | Global options to options
|
9313ce8 | Recasting options as attributes
|
11a13f4 | Global options as class attributes
|
217b5b4 | Fixing doctests
|
b761fec | Merging into 7.3.beta4
|
caabdb0 | Fixing more doc-tests
|
11b7080 | More doc-tests
|
comment:12 in reply to: ↑ 11 Changed 5 years ago by
Replying to git:
This was to remove 20618 from the commit tree as it shouldn't have been there...
comment:13 Changed 5 years ago by
- Branch changed from u/andrew.mathas/pickling_global_options to public/misc/pickling_global_options-18555
- Commit changed from 11b7080fb16f62f64d39bca2286d0912c55a2125 to f42beb4a800cf4fd9c07acfbd054fd91b896edb2
- Keywords days78 added
- Status changed from new to needs_review
Okay, I fixed the doctest failures with permutation.py
. The issue was that the _Option
(now Option
so it appears in the doc) did not define a __ne__
method, and so it returned True
for things like opt != X
even when opt == X
.
The other doctest failures were due to lack of conversion to options
from global_options
.
This is a very nice improvement over the current interface to (global) options. If the doctest pass for you and my changes are good, then you can set this to a positive review.
New commits:
0636c87 | Merge branch 'u/andrew.mathas/pickling_global_options' of trac.sagemath.org:sage into public/misc/pickling_global_options-18555
|
9642057 | Making everything work with the new API.
|
f42beb4 | Rename _Option to Option and some PEP8 to global_options.py.
|
comment:14 Changed 5 years ago by
- Commit changed from f42beb4a800cf4fd9c07acfbd054fd91b896edb2 to 4013abb4bbea7687da60f3240127027475d5e410
Branch pushed to git repo; I updated commit sha1. New commits:
4013abb | Fixing failing doctests identified by the patchbot
|
comment:15 Changed 5 years ago by
There's some doc issues in the manifold.py
options: EXAMPLES::
and everything including and after sage: latex(f)
is over indented.
comment:16 Changed 5 years ago by
- Commit changed from 4013abb4bbea7687da60f3240127027475d5e410 to 3f5e0e91e2289dcee18ad56b30c6884f9d26c890
Branch pushed to git repo; I updated commit sha1. New commits:
3f5e0e9 | Fixing manifold documemtation issues and collecting all deprecation messages at the end of the files
|
comment:17 Changed 5 years ago by
- Description modified (diff)
Thanks Travis. Fixed. I have also cleaned up the deprecation messages, putting them in the same place, so that they will be easier to remove in future.
Finally, should the reset
method be _reset()
or Reset()
? Currently it is _reset()
but it probably shouldn't be hidden.
comment:18 Changed 5 years ago by
- Milestone changed from sage-6.8 to sage-7.3
Reset()
breaks python conventions, so that is a -1 from me. I think reset()
or _reset()
are both fine. Yet there is not so much a good general reason to go back to defaults other than for testing purposes in my mind. Hence the hidden _reset()
is better IMO.
One last thing, you added an __init__
method in interval_poset.py
. Is this necessary? If so, it needs a doctest. Once this is done, this will be at a positive review.
comment:19 Changed 5 years ago by
- Commit changed from 3f5e0e91e2289dcee18ad56b30c6884f9d26c890 to 8322fd5fe18b1bb624b78fbeccf2a8c37bd5e69f
Branch pushed to git repo; I updated commit sha1. New commits:
8322fd5 | Removing __init__ from interval_poset.py
|
comment:20 Changed 5 years ago by
- Status changed from needs_review to positive_review
Fixed -- not sure how that got in there. Thanks Travis!
comment:21 Changed 5 years ago by
- Status changed from positive_review to needs_work
Reviewer name is missing
comment:22 Changed 5 years ago by
- Reviewers set to Travis Scrimshaw
- Status changed from needs_work to positive_review
comment:23 Changed 5 years ago by
- Status changed from positive_review to needs_work
Merge conflict, try next beta
comment:24 Changed 5 years ago by
- Commit changed from 8322fd5fe18b1bb624b78fbeccf2a8c37bd5e69f to ed5070dd7ed52b0cbb405cb77cb651fb6d6f2f38
Branch pushed to git repo; I updated commit sha1. New commits:
ed5070d | Merging 7.3.beta7
|
comment:25 Changed 5 years ago by
- Status changed from needs_work to positive_review
Merged into 7.3.beta7. Tests pass.
comment:26 Changed 5 years ago by
- Status changed from positive_review to needs_work
Fails on 32-bit
sage -t --long src/sage/structure/global_options.py ********************************************************************** File "src/sage/structure/global_options.py", line 599, in sage.structure.global_options.Option.__hash__ Failed example: hash( Tableaux.options.convention ) # indirect doc-test Expected: -6673246059928475433 Got: -1691342633 ********************************************************************** 1 item had failures: 1 of 2 in sage.structure.global_options.Option.__hash__ [135 tests, 1 failure, 0.03 s]
comment:27 follow-up: ↓ 29 Changed 5 years ago by
This just needs to mark the different output as # 32-bit
and # 64-bit
, but I'd change the actual test to hash(Tableaux.options.convention) == hash(Tableaux.options('convention')
(it also doesn't need the # indirect doctest
marker). Sorry for not catching that sooner.
comment:28 Changed 5 years ago by
- Commit changed from ed5070dd7ed52b0cbb405cb77cb651fb6d6f2f38 to ba7dd9836386ed119c257d9bb42030149e091b47
Branch pushed to git repo; I updated commit sha1. New commits:
ba7dd98 | Fixing hashed doct-test
|
comment:29 in reply to: ↑ 27 Changed 5 years ago by
- Status changed from needs_work to positive_review
Replying to tscrim:
This just needs to mark the different output as
# 32-bit
and# 64-bit
, but I'd change the actual test tohash(Tableaux.options.convention) == hash(Tableaux.options('convention')
(it also doesn't need the# indirect doctest
marker). Sorry for not catching that sooner.
Thanks Travis! Fixed and changed back to a positive review.
comment:30 Changed 5 years ago by
- Branch changed from public/misc/pickling_global_options-18555 to ba7dd9836386ed119c257d9bb42030149e091b47
- Resolution set to fixed
- Status changed from positive_review to closed
As I am cleaning this up a related question comes to mind: the name
GlobalOptions
is slightly unfortunate because instances of this class are really just options for an associated family of objects. I can sort of forgive myself for using this name for the class, but in all of the instances of this class the associated "parent" classes have aglobal_options
method that points back to its options class. This method is now being enshrined more firmly inside the code because it is exploited to allow pickling. I would be happier if these methods were simply calledoptions
, rather thanglobal_options
, even though this change would necessitate deprecating the existingglobal_options
methods.Thoughts?