Replace sage.algebras.yangian.GeneratorIndexingSet with cartesian_product

[mkoeppe]

We replace the GeneratorIndexingSet in yangian.py with cartesian_product, which its implementation is (now) duplicating.

CC: @tscrim

Component: algebra

Author: Travis Scrimshaw

Branch/Commit: abf1edc

Reviewer: Matthias Koeppe

Issue created by migration from https://trac.sagemath.org/ticket/34375

[mkoeppe]

Branch: u/mkoeppe/sage_algebras_yangian_generatorindexingset_should_be_a_parent

[mkoeppe]

Author: Matthias Koeppe

[mkoeppe]

Commit: b96983a

[mkoeppe]

New commits:

b96983a

src/sage/algebras/yangian.py: Make GeneratorIndexingSet a Parent

[tscrim]

comment:3

This probably can now just be replaced by cartesian_product. I believe when I wrote this, it was not as good as it is now. I don't think the façade for tuples property is so important (in particular, I don't think it makes any real difference in speed).

[mkoeppe]

comment:4

Probably, but I'm not inclined to work on that

[mkoeppe]

comment:5

(edit: NOT inclined)

[tscrim]

comment:6

Actually, I am starting to get a little worried about this (AFAIK implicit) requirement that every input to index a CFM basis (or perhaps more generally that anything that could potentially model a set within Sage that the user could potentially get to using public functions) needs to be a Parent. It is more restrictive than before and could potentially have knock-on effects, including speed regressions (an element can be a heavy object to construct compared to, e.g., a tuple and have slower hashing).

I would say the bug described in the ticket is really a fault of Set and not of the GeneratorIndexingSet (which clearly knows it is infinite through cardinality()).

Hence, I think this ticket as-is needs more justification than what has been given.

That being said, would be good here to avoid code duplication with cartesian_product as I described in comment:3.

[mkoeppe]

comment:7

Replying to @tscrim:

this (AFAIK implicit) requirement that every input to index a CFM basis (or perhaps more generally that anything that could potentially model a set within Sage that the user could potentially get to using public functions) needs to be a Parent.

Nobody is making or proposing this change

[mkoeppe]

comment:8

Your GeneratorIndexingSet is an infinite generator without advertising that it's infinite. Which is inconvient

[tscrim]

comment:9

sage: from sage.algebras.yangian import GeneratorIndexingSet
sage: I = GeneratorIndexingSet((1,2))
sage: I.cardinality()
+Infinity

[mkoeppe]

comment:10

A duck-typed cardinality method, I see.

[tscrim]

comment:11

Replying to @mkoeppe:

Replying to @tscrim:

this (AFAIK implicit) requirement that every input to index a CFM basis (or perhaps more generally that anything that could potentially model a set within Sage that the user could potentially get to using public functions) needs to be a Parent.

Nobody is making or proposing this change

That is the by-product of saying that GeneratorIndexingSet must be a Parent subclass. What about any other (infinite) set that is not a Parent?

[mkoeppe]

comment:12

I ran into this problem only because we a _test_as_set_object method in the category.
This calls Set and then runs tests on that. There's no problem otherwise. If you don't want your GeneratorIndexingSet put in a class/category where cardinality actually means something, we can just disable this TestSuite test

[tscrim]

comment:13

I think that is a bad test. It exactly is (somewhat implicitly) giving the specification I said in comment:6. Why is that implementation of Set special that it must work for that? What about for objects that are not supposed to model sets? It should be a bad thing to skip tests, usually because some functionality is not implemented. I don't like allowing such a broad exception.

[mkoeppe]

comment:14

_test_as_set_object is a method of Set_base

[tscrim]

comment:15

Then I don't understand the problem. The TestSuite(I).run() should still pass. It sounds like this ticket should either be a wontfix or could be changed into the comment:3 proposal. I would say it is the fault of Set(I) of not handling cases correctly or this falls under the undecidable problem of determining if a general iterator is infinite ahead of time.

[mkoeppe]

comment:16

Replying to @tscrim:

this falls under the undecidable problem of determining if a general iterator is infinite ahead of time.

Exactly, which is why it's not enough just to give something a cardinality method and hope that someone hears it quack. Best to make it a proper enumerated set, not a naked UniqueRepresentation.

[tscrim]

comment:17

Replying to @mkoeppe:

Replying to @tscrim:

this falls under the undecidable problem of determining if a general iterator is infinite ahead of time.

Exactly, which is why it's not enough just to give something a cardinality method and hope that someone hears it quack. Best to make it a proper enumerated set, not a naked UniqueRepresentation.

I think it is foolhardy to enable one specific implementation to dictate what all other classes must do, independent of what their intended purpose is for (a la comment:6).

[mkoeppe]

comment:18

Which "implementation" are you referring to?

[tscrim]

comment:19

Replying to @mkoeppe:

Which "implementation" are you referring to?

The implementation of Set.

[mkoeppe]

comment:20

This file clearly needs to be updated using the infrastructure of EnumeratedSets. Smelly duck typing going on there with details such as cardinality returning an int, not an Integer.

[tscrim]

comment:21

I really don't think we should mandate that it needs to be an EnumeratedSet (basically comment:6). However, it is bad that cardinality() can return an Integer when used within Sage (which I cannot reproduce without specifying that the level is an int).

[mkoeppe]

comment:68

Replying to @tscrim:

Replying to @mkoeppe:

Yes, by providing this special method __len__, a class promises to follow the Sized protocol.

So anything that implements __len__, no matter what it does, must do what behaviors?

The obvious ones. No language-lawyering allowed.

[tscrim]

comment:69

Replying to @mkoeppe:

Replying to @tscrim:

Replying to @mkoeppe:

The semantics of Sized is guaranteed because of presence of __len__.

There are no semantics that have been specified. By your own admission, there are none in the Python (ABC) doc.

You are still missing my point about this not being a formal specification.

It's invalid to say "haha, I can do anything I want because it's not explicitly forbidden".

It's also invalid to say "Because it doesn't follow my arbitrary specifications of what I think is good that you cannot do it."

This is a valid approach with a formal specification.

It is meaningless with the Python docs.

Then please stop citing and using them as specifications or as a reason why we should do certain things.

[tscrim]

comment:70

Replying to @mkoeppe:

One has to ask: What were the intentions in defining these ABCs?

And the answer is certainly not to say "these are good-sounding names, but they guarantee NOTHING!"

Indeed, it is sayin "if I am a subclass of this ABC, then I will implement the specifications of this ABC". In this case, it is just that these methods are provided.

[tscrim]

comment:71

Replying to @mkoeppe:

Replying to @tscrim:

Replying to @mkoeppe:

Yes, by providing this special method __len__, a class promises to follow the Sized protocol.

So anything that implements __len__, no matter what it does, must do what behaviors?

The obvious ones. No language-lawyering allowed.

Which are?

[mkoeppe]

comment:72

Replying to @tscrim:

Replying to @mkoeppe:

One has to ask: What were the intentions in defining these ABCs?

And the answer is certainly not to say "these are good-sounding names, but they guarantee NOTHING!"

Indeed, it is sayin "if I am a subclass of this ABC, then I will implement the specifications of this ABC". In this case, it is just that these methods are provided.

You are still missing that for these special ABCs there are 3 additional special mechanisms that make a class a subclass of the ABC. I've already quoted them for you in comment:54

[mkoeppe]

comment:73

I recommend to read the PEP that introduced these things - https://peps.python.org/pep-3119/#abcs-for-containers-and-iterators

[tscrim]

comment:74

Replying to @mkoeppe:

Replying to @tscrim:

Replying to @mkoeppe:

One has to ask: What were the intentions in defining these ABCs?

And the answer is certainly not to say "these are good-sounding names, but they guarantee NOTHING!"

Indeed, it is sayin "if I am a subclass of this ABC, then I will implement the specifications of this ABC". In this case, it is just that these methods are provided.

You are still missing that for these special ABCs there are 3 additional special mechanisms that make a class a subclass of the ABC. I've already quoted them for you in comment:54

I am wondering if we have a slightly different definition of the words that they are using in the doc. We are disagreeing on the existence of something in that doc. The PEP you just cited actually has specifications. However, that is about something that is an explicit subclass. The isinstance test here is wrong.

[mkoeppe]

comment:75

I think you'll have to take a bit of time reading it.
It's my bedtime anyway, so there's plenty of time.

[tscrim]

comment:76

To guarantee that the behaviors specified in the PEP are followed, you should use issubclass rather than isinstance to check it if you don't want false-positives (as it is just checking if the method is there). As I mentioned, not everything that implements a __len__ should be considered an element of collections.abc.Sized.

[mkoeppe]

comment:77

No, that does not make any sense.

[mkoeppe]

Reviewer: Matthias Koeppe

[mkoeppe]

comment:78

But in the meantime, the bots have all turned green. We can continue the discussion about containers.abc on another ticket.

[vbraun]

comment:79

sage -t --long --warn-long 49.3 --random-seed=123 src/sage/algebras/yangian.py
    Timed out
**********************************************************************
Tests run before process (pid=4057225) timed out:
sage: Y = Yangian(QQ, 4) ## line 152 ##
...
sage: grY = Yangian(QQ, 4, filtration='natural').graded_algebra() ## line 973 ##
sage: x = grY.gen(12, 2, 1) * grY.gen(2, 1, 1) # indirect doctest ## line 974 ##
sage: x ## line 975 ##
tbar(2)[1,1]*tbar(12)[2,1]
sage: x == grY.gen(2, 1, 1) * grY.gen(12, 2, 1) ## line 977 ##
True
sage: sig_on_count() # check sig_on/off pairings (virtual doctest) ## line 979 ##
0
sage: grY = Yangian(QQ, 4).graded_algebra() ## line 1003 ##
sage: TestSuite(grY).run()  # long time ## line 1004 ##
------------------------------------------------------------------------

[sagetrac-git]

Changed commit from a346bff to abf1edc

[sagetrac-git]

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

abf1edc

Choosing smaller degree elements to run the test suite.

[tscrim]

comment:81

The generic element for the new indices was too big. I explicitly pass in a few elements for testing. It now passes in a little over a second for me (on my fairly fast computer).

[vbraun]

Changed branch from public/algebras/yangian_index_set-34375 to abf1edc