Opened 4 years ago
Last modified 4 years ago
#22965 needs_work enhancement
Refactor axioms
Reported by:  nthiery  Owned by:  

Priority:  major  Milestone:  sage8.0 
Component:  categories  Keywords:  
Cc:  tscrim, SimonKing, vbraun, jpflori  Merged in:  
Authors:  Nicolas M. Thiéry  Reviewers:  
Report Upstream:  N/A  Work issues:  
Branch:  u/nthiery/refactor_axioms (Commits, GitHub, GitLab)  Commit:  f4ce06aa2d0449e1ac4d33b1031f19b07a15dfb0 
Dependencies:  Stopgaps: 
Description (last modified by )
This ticket brings:
 a catalog of axioms
axioms.XXX
in the global namespace
 axioms as objects rather than plain strings. This had been wished for during the review #10963, and some of the code is reminiscent of Volker's draft at #15501. The main advantage for now is to enable the definition of axioms with custom printing, thus getting rid of the crapy monolithic customization section.
The bijection axiom <> "axiom name" remains mandatory to support the syntax for implementing axioms in a category.
 improved heuristic for printing categories with axioms. E.g.:
Category of finite enumerated fields > Category of finite fields Category of finite enumerated permutation groups > Category of finite permutation groups
improved printing of
AssociativeXXX
:Category of additive commutative additive magmas > Category of additivecommutative additive magmasThis was discussed and requested on sagedevel for better readability when there are many axioms printed. E.g. in
sage: Semirings().super_categories()[0] Category of associative additivecommutative additiveassociative additiveunital distributive magmas and additive magmas
The above are the reasons why the commits touch many files)
 some simplifications in the code (e.g. no more
_repr_object_names_static
)
The code is essentially backward compatible, at the price of having
axioms.Associative == "Associative"
, with compatible hash values.
Using axioms as strings is deprecated in code. In particular, the
ticket updates the Sage library in a few places to use the preferred
idiom P in C.Axiom
rather than "Axiom" in P.category().axioms()
.
Remaining questions:
 Inside the code, do we want to call the catalog of axioms
all_axioms
oraxioms
? The latter is more ambiguous, but consistent with the name exported in global name space.
 To fetch an existing axiom from a variable containing either the
axiom or its name, the current idiom is
all_axioms(name)
; that's somewhat consistent with theP(xx)
idiom to create elements of a parent. Would we want some other idiom?all_axioms[name]
?
 Do we want to implement a
cmp
method for axioms? Comparing by string: this could avoid the repeated use of
sorted(...,key=str)
in our documentation  Comparing by index/definition time? This would be plausibly more meaningful (axioms shown in some natural order). And simplify a very tiny bit the one place where this sorting order is used in the code.
 Comparing by string: this could avoid the repeated use of
 Do we want to Cythonize axiom.py? Now? Later?
 Do we want to import
all_axioms
fromcategory_with_axiom
rather thanaxiom
? This would avoid a double import in all the files specifying_base_category_and_axiom
 Do we want "additive magmas" to be renamed to "additivemagmas" for consistency?
 Do we want to fix the inconsistency below by adding
additive
in the first output?
sage: CommutativeAdditiveGroups() Category of commutative additive groups sage: AdditiveMagmas().AdditiveCommutative() Category of additivecommutative additive magmas
 To recover the string associated to an axiom in the code, is it fine
to use
str(axiom)
, or do we want something more explicit and possibly distinct from printing likeaxiom.name()
?
 Any better name for
axiom.index()
? It's only used internally, and in a single place; we could also just useaxiom._index
.
Work for followup tickets:
 Improve the guessing to avoid having to specify
_base_category_class_and_axiom
inFiniteDimensionalLieAlgebrasWithBasis
and friends.
 Improve the pretty printing to get:
Category of associative additivecommutative additiveassociative additiveunital distributive magmas and additive magmas > Category of distributive semigroups and additivecommutative additive monoids
Change History (27)
comment:1 Changed 4 years ago by
 Description modified (diff)
comment:2 Changed 4 years ago by
 Cc tscrim added
 Description modified (diff)
comment:3 Changed 4 years ago by
 Branch set to u/nthiery/refactor_axioms
comment:4 followup: ↓ 16 Changed 4 years ago by
 Commit set to 4b21055f4282d23aee0bf929c5d0374a748af32c
comment:5 Changed 4 years ago by
 Commit changed from 4b21055f4282d23aee0bf929c5d0374a748af32c to 47d3b7ca29a764845c4732569ed86bff1626d1e4
comment:6 Changed 4 years ago by
 Commit changed from 47d3b7ca29a764845c4732569ed86bff1626d1e4 to c4e0fb555fb64ef6d121a2a368dfed117844ff6d
Branch pushed to git repo; I updated commit sha1. New commits:
c4e0fb5  22965: idiom update: use P in C.Axiom rather than "Axiom" in P.category().axioms()

comment:7 Changed 4 years ago by
 Commit changed from c4e0fb555fb64ef6d121a2a368dfed117844ff6d to efedd94779391402f3138b23a6dbf15888514867
Branch pushed to git repo; I updated commit sha1. New commits:
efedd94  22965: documentation updates

comment:8 Changed 4 years ago by
 Commit changed from efedd94779391402f3138b23a6dbf15888514867 to 0f2464d0104ad87319519c94daf3dcc5ab93dae9
Branch pushed to git repo; I updated commit sha1. New commits:
0f2464d  22965: refactorisation of axioms (code part: hunks missing from previous commit)

comment:9 Changed 4 years ago by
 Commit changed from 0f2464d0104ad87319519c94daf3dcc5ab93dae9 to 75ea783992558b4b422ca4cf71f06f376c821ca5
Branch pushed to git repo; I updated commit sha1. New commits:
75ea783  22965: Trivial doctest updates ('XXX' > XXX, nicer category names)

comment:10 Changed 4 years ago by
 Cc SimonKing added
 Description modified (diff)
 Status changed from new to needs_review
comment:11 Changed 4 years ago by
 Description modified (diff)
comment:12 followup: ↓ 14 Changed 4 years ago by
If you're going to override "eq" and "hash" you should probably define the thing to be CachedRepresentation
. Is there a good reason to be CachedRepresentation
?
(CachedRepresentation
has a very nontrivial cost. You should only use it if you need its features)
comment:13 Changed 4 years ago by
 Description modified (diff)
comment:14 in reply to: ↑ 12 Changed 4 years ago by
Replying to nbruin:
If you're going to override "eq" and "hash" you should probably define the thing to be
CachedRepresentation
. Is there a good reason to beCachedRepresentation
?(
CachedRepresentation
has a very nontrivial cost. You should only use it if you need its features)
That's a good question. The __eq__
and __hash__
are really here just for backward compatibility (supporting the idiom "Foo" in C.axioms()
). I used UniqueRepresentation
because that's really what I mean :) But you are right, it could be more proper to use CachedRepresentation
.
In terms of speed, I believe the only important thing is fast equality and hash (which calls for Cythonizing them). Axioms will rarely be recreated.
Cheers,
Nicolas
comment:15 Changed 4 years ago by
 Cc vbraun added
comment:16 in reply to: ↑ 4 Changed 4 years ago by
Replying to tscrim:
I know it's not ready for review, but something I've been bit by in the past is only defining an
__eq__
. Make sure you define a__ne__
asreturn not (self == other)
.
Yeah, I pondered about this. Accepting "Associative" == axioms.Associative
is really just here as a workaround for backward compatibility; namely to support idiom "Associative" in C.axioms()
that people apparently have been using. I wondered whether I really wanted to enable more than just the strict minimum.
comment:17 Changed 4 years ago by
If you want to keep the __eq__
, then you really need to define a __ne__
to keep things consistent in case someone wants to test inequality. A slightly contrived example, but say someone has a constructor that accepts an axiom as an arg and they want to do something special when the axiom is not axioms.Commutative
.
Also, __init__
, __repr__
, and index
of Axiom
do not have doctests.
comment:18 Changed 4 years ago by
I'm not really commenting on whether you should define an __eq__
or not.
comment:19 followup: ↓ 21 Changed 4 years ago by
Also, why does Axiom
inherit from SageObject
? What features are you using?
comment:20 Changed 4 years ago by
 Commit changed from 75ea783992558b4b422ca4cf71f06f376c821ca5 to 126bb507aeef646d4535622aa3b0ea998042d29c
Branch pushed to git repo; I updated commit sha1. New commits:
126bb50  22965: fixed missing doctests

comment:21 in reply to: ↑ 19 Changed 4 years ago by
Replying to tscrim:
Also, why does
Axiom
inherit fromSageObject
? What features are you using?
I believe none at this stage. I just put that by default, mostly for consistency with Category.
comment:22 Changed 4 years ago by
 Commit changed from 126bb507aeef646d4535622aa3b0ea998042d29c to f4ce06aa2d0449e1ac4d33b1031f19b07a15dfb0
Branch pushed to git repo; I updated commit sha1. New commits:
f4ce06a  22965: added Axiom.__ne__ as negation of Axiom.__eq__

comment:23 Changed 4 years ago by
 Description modified (diff)
comment:24 Changed 4 years ago by
Hi; any comment on the questions raised in the description?
comment:25 Changed 4 years ago by
 Inside the code, do we want to call the catalog of axioms
all_axioms
oraxioms
? The latter is more ambiguous, but consistent with the name exported in global name space.
I say axioms
both for consistency and naturality.
 To fetch an existing axiom from a variable containing either the axiom or its name, the current idiom is
all_axioms(name)
; that's somewhat consistent with theP(xx)
idiom to create elements of a parent. Would we want some other idiom?all_axioms[name]
?
I would go for the latter since it is a container object and all_axioms[name]
is how Python handles other container objects, e.g., dict
.
 Do we want to implement a
cmp
method for axioms?
No, but rich comparisons are good.
 Comparing by string: this could avoid the repeated use of
sorted(...,key=str)
in our documentation Comparing by index/definition time? This would be plausibly more meaningful (axioms shown in some natural order). And simplify a very tiny bit the one place where this sorting order is used in the code.
Definitely Index/definition time as this will allow us to control the printing in a more straightforward way.
 Do we want to Cythonize axiom.py? Now? Later?
I am inclined to say Cythonize it to keep the categories as fast as possible. I feel like even if highly optimized, this switch will slow down category creation because the string operations are lightweight and optimized at the machine level. However, I haven't done any timings.
Also, it would be good if you could get rid of the SageObject
inheritance from Axiom
so we could cdef
it, and I don't see axioms fundamentally behaving like other instances of SageObject
.
 Do we want to import
all_axioms
fromcategory_with_axiom
rather thanaxiom
? This would avoid a double import in all the files specifying_base_category_and_axiom
 Do we want "additive magmas" to be renamed to "additivemagmas" for consistency?
I don't think the hyphen there is completely grammatically correct, at least it is strange to me. So I'm 1.
 Do we want to fix the inconsistency below by adding
additive
in the first output?sage: CommutativeAdditiveGroups() Category of commutative additive groups sage: AdditiveMagmas().AdditiveCommutative() Category of additivecommutative additive magmas
I would like the latter to be
Category of commutative additive magmas
as this is more concise and easier to parse.
 To recover the string associated to an axiom in the code, is it fine to use
str(axiom)
, or do we want something more explicit and possibly distinct from printing likeaxiom.name()
?
str(axiom)
is the most natural for me, but it doesn't hurt to have an alias axiom.name()
.
 Any better name for
axiom.index()
? It's only used internally, and in a single place; we could also just useaxiom._index
.
I would get rid of it all together and (mildly) break encapsulation here by using axiom._index
. It is both faster, only done in one place, and is an implementation detail.
comment:26 Changed 4 years ago by
 Cc jpflori added
comment:27 Changed 4 years ago by
 Status changed from needs_review to needs_work
Looks like there are a lot of things in the comments to still be addressed and merge conflicts that need to be resolved.
I know it's not ready for review, but something I've been bit by in the past is only defining an
__eq__
. Make sure you define a__ne__
asreturn not (self == other)
.New commits:
22965: refactorisation of axioms (code part)