Opened 4 years ago

Last modified 4 years ago

#22965 needs_work enhancement

Refactor axioms

Reported by: nthiery Owned by:
Priority: major Milestone: sage-8.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:

Status badges

Description (last modified by nthiery)

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 additive-commutative additive magmas

This was discussed and requested on sage-devel for better readability when there are many axioms printed. E.g. in

sage: Semirings().super_categories()[0]
Category of associative additive-commutative additive-associative additive-unital 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 or axioms? 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 the P(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.
  • Do we want to Cythonize axiom.py? Now? Later?
  • Do we want to import all_axioms from category_with_axiom rather than axiom? This would avoid a double import in all the files specifying _base_category_and_axiom
  • Do we want "additive magmas" to be renamed to "additive-magmas" 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 additive-commutative 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 like axiom.name()?
  • Any better name for axiom.index()? It's only used internally, and in a single place; we could also just use axiom._index.

Work for followup tickets:

  • Improve the guessing to avoid having to specify _base_category_class_and_axiom in FiniteDimensionalLieAlgebrasWithBasis and friends.
  • Improve the pretty printing to get:
    Category of associative additive-commutative additive-associative additive-unital distributive magmas and additive magmas
    ->
    Category of distributive semigroups and additive-commutative additive monoids
    

Change History (27)

comment:1 Changed 4 years ago by nthiery

  • Description modified (diff)

comment:2 Changed 4 years ago by tscrim

  • Cc tscrim added
  • Description modified (diff)

comment:3 Changed 4 years ago by nthiery

  • Branch set to u/nthiery/refactor_axioms

comment:4 follow-up: Changed 4 years ago by tscrim

  • Commit set to 4b21055f4282d23aee0bf929c5d0374a748af32c

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__ as return not (self == other).


New commits:

4b2105522965: refactorisation of axioms (code part)

comment:5 Changed 4 years ago by git

  • Commit changed from 4b21055f4282d23aee0bf929c5d0374a748af32c to 47d3b7ca29a764845c4732569ed86bff1626d1e4

Branch pushed to git repo; I updated commit sha1. New commits:

af59aff22965: Trivial doctest updates ('XXX' -> XXX, nicer category names)
47d3b7c22965: updates in the categories to use axioms rather than strings

comment:6 Changed 4 years ago by git

  • Commit changed from 47d3b7ca29a764845c4732569ed86bff1626d1e4 to c4e0fb555fb64ef6d121a2a368dfed117844ff6d

Branch pushed to git repo; I updated commit sha1. New commits:

c4e0fb522965: idiom update: use P in C.Axiom rather than "Axiom" in P.category().axioms()

comment:7 Changed 4 years ago by git

  • Commit changed from c4e0fb555fb64ef6d121a2a368dfed117844ff6d to efedd94779391402f3138b23a6dbf15888514867

Branch pushed to git repo; I updated commit sha1. New commits:

efedd9422965: documentation updates

comment:8 Changed 4 years ago by git

  • Commit changed from efedd94779391402f3138b23a6dbf15888514867 to 0f2464d0104ad87319519c94daf3dcc5ab93dae9

Branch pushed to git repo; I updated commit sha1. New commits:

0f2464d22965: refactorisation of axioms (code part: hunks missing from previous commit)

comment:9 Changed 4 years ago by git

  • Commit changed from 0f2464d0104ad87319519c94daf3dcc5ab93dae9 to 75ea783992558b4b422ca4cf71f06f376c821ca5

Branch pushed to git repo; I updated commit sha1. New commits:

75ea78322965: Trivial doctest updates ('XXX' -> XXX, nicer category names)

comment:10 Changed 4 years ago by nthiery

  • Cc SimonKing added
  • Description modified (diff)
  • Status changed from new to needs_review

comment:11 Changed 4 years ago by nthiery

  • Description modified (diff)

comment:12 follow-up: Changed 4 years ago by 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 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 nthiery

  • Description modified (diff)

comment:14 in reply to: ↑ 12 Changed 4 years ago by nthiery

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 be CachedRepresentation?

(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 nthiery

  • Cc vbraun added

comment:16 in reply to: ↑ 4 Changed 4 years ago by nthiery

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__ as return 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 tscrim

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 tscrim

I'm not really commenting on whether you should define an __eq__ or not.

comment:19 follow-up: Changed 4 years ago by tscrim

Also, why does Axiom inherit from SageObject? What features are you using?

comment:20 Changed 4 years ago by git

  • Commit changed from 75ea783992558b4b422ca4cf71f06f376c821ca5 to 126bb507aeef646d4535622aa3b0ea998042d29c

Branch pushed to git repo; I updated commit sha1. New commits:

126bb5022965: fixed missing doctests

comment:21 in reply to: ↑ 19 Changed 4 years ago by nthiery

Replying to tscrim:

Also, why does Axiom inherit from SageObject? 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 git

  • Commit changed from 126bb507aeef646d4535622aa3b0ea998042d29c to f4ce06aa2d0449e1ac4d33b1031f19b07a15dfb0

Branch pushed to git repo; I updated commit sha1. New commits:

f4ce06a22965: added Axiom.__ne__ as negation of Axiom.__eq__

comment:23 Changed 4 years ago by nthiery

  • Description modified (diff)

comment:24 Changed 4 years ago by nthiery

Hi; any comment on the questions raised in the description?

comment:25 Changed 4 years ago by tscrim

  • Inside the code, do we want to call the catalog of axioms all_axioms or axioms? 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 the P(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 from category_with_axiom rather than axiom? This would avoid a double import in all the files specifying _base_category_and_axiom
  • Do we want "additive magmas" to be renamed to "additive-magmas" 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 additive-commutative 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 like axiom.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 use axiom._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 jpflori

  • Cc jpflori added

comment:27 Changed 4 years ago by mderickx

  • 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.

Note: See TracTickets for help on using tickets.