cardinality must output Infinty or a Sage integer

[vdelecroix]

We add a ._test_cardinality() method in the category Sets that takes care of checking that the output of .cardinality() is a Sage integer or +Infinity.

[vdelecroix]

New commits:

​28378f0

Trac 18159: add a ._test_cardinality in Sets category

[git]

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

​17924b8

Trac 18159: fix cardinality/is_finite in sets/set.py

[ncohen]

What is wrong with an 'int'? O_o

[vdelecroix]

Replying to ncohen:

What is wrong with an 'int'? O_o

2/3 for example.

[ncohen]

I do not understand. It seems that your code detects it as 'wrong' if a function .cardinality() returns int(4). Why is that?

[vdelecroix]

Replying to ncohen:

I do not understand. It seems that your code detects it as 'wrong' if a function .cardinality() returns int(4). Why is that?

Yes. And the reason is because Python int do not behave as Sage integers. For example

sage: S = my_finite_set()
sage: S.cardinality().factor()
sage: ~(S.cardinality())

For coherence, we need to have some specifications for .cardinality.

The main reason for that ticket was that some sets were actually returning rational numbers see ​http://trac.sagemath.org/ticket/17852#comment:92

[ncohen]

That they behave differently is not (to me) a sufficient reason to prevent everybody from returning python ints. Checking that the output can be turned into an int would be sufficient to detect this problem with rational numbers.

[vdelecroix]

Replying to ncohen:

That they behave differently is not (to me) a sufficient reason to prevent everybody from returning python ints. Checking that the output can be turned into an int would be sufficient to detect this problem with rational numbers.

All right, output string is accepted as well?

This is very bad. Imagine I need something like:

sage: vector([s.cardinality() for s in S])

If any of the s.cardinality() returns a rational, I end up with a vector over rational numbers (which are much slower than over integers).

It is very similar to is_X functions that return boolean answers. You might complain if they start returning gap booleans or numpy boolean objects.

Vincent

[ncohen]

What would you have against testing that it is an int, a long, an Integer, or equal to (not 'is') infinity?

Nathann

[vdelecroix]

Replying to ncohen:

What would you have against testing that it is an int, a long, an Integer, or equal to (not 'is') infinity?

You do not want to see a Sage rational anymore? And what about numpy integers, pari integers, gp integers, gap integers and libgap integers?

Testing equality to infinity might be broken or you can have objects that want to be equal to everybody (why not?).

The specification in sage.categories.sets_cat is:

``self.cardinality()`` should return the cardinality of the set
``self`` as a sage :class:`Integer` or as ``infinity``.

If you complain that it should not be the case then you should argue why. And not telling me to argue why not.

Vincent

[ncohen]

I think that it should not be the case because python ints/long are smaller and faster than Sage Integers, and that we should not be forced to instantiate Integers if we do not need to.

For the very same reason you invoked, earlier, when you said that you did not want rationals because of the possible loss of time.

Nathann

[ncohen]

(note that your problems are solved if you specify ZZ when you build your vector, isn't it?)

[vdelecroix]

Replying to ncohen:

I think that it should not be the case because python ints/long are smaller and faster than Sage Integers, and that we should not be forced to instantiate Integers if we do not need to.

For the very same reason you invoked, earlier, when you said that you did not want rationals because of the possible loss of time.

It was not only loss of time but well defineness. Additioning Python int with Sage integers is slower than Sage integer with Sage integer.

sage: python_one = 1r
sage: sage_one = 1
sage: sage_a = 2**12
sage: timeit("sage_a + python_one", number=5000000)
5000000 loops, best of 3: 66.3 ns per loop
sage: timeit("sage_a + sage_one", number=5000000)
5000000 loops, best of 3: 57.9 ns per loop

So it is better to have everything being Sage integers.

On the other hand, if you really want Python integers you should call len(my_set) (and be sure to obtain a Python integer) and not my_set.cardinality().

Vincent

[vdelecroix]

Replying to ncohen:

(note that your problems are solved if you specify ZZ when you build your vector, isn't it?)

Yes, and

sage: vector(ZZ, ['1', gap(2), pari(3)])
(1, 2, 3)

works too.

[git]

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

​e40975a

Trac 18159: fix some tests related to TestSuite

[ncohen]

So it is better to have everything being Sage integers.

It may be better to have Sage integers if you want, for instance, to add them with other Sage integers. Why would that be sufficient to prevent anyone from returning int?

Nathann

[ncohen]

Okay do whatever you like. At some point in this community there was a rule that we should all agree on something to make it pass. Nowadays it's like you just have to convince one of your friends and it can get in.

Nathann

[vdelecroix]

Replying to ncohen:

So it is better to have everything being Sage integers.

It may be better to have Sage integers if you want, for instance, to add them with other Sage integers. Why would that be sufficient to prevent anyone from returning int?

It is not sufficient. You were arguing that Python ints were better because faster. I just showed you that it is not completely right. An argument for X is generally not an argument for Y.

The reason to oblige people to return Sage integers or Infinity in the method .cardinality of a Parent is mostly for consistency (things are generally better if specified)

[ncohen]

It is not sufficient. You were arguing that Python ints were better because faster. I just showed you that it is not completely right. An argument for X is generally not an argument for Y.

All I wanted to point out is that *sometimes* an int is better than an Integer, and that for this reason they should not be forbidden. I do not mean to say that a Python int is always better than a Sage Integer, and quite clearly it is not the case: ints overflows, Integers do not.

The reason to oblige people to return Sage integers or Infinity in the method .cardinality of a Parent is mostly for consistency.

Couldn't we just use the TestSuite to detect errors? When you see a non-integer then something is most probably wrong and the testsuite should report it. What you are doing right now is simulate a "return type" for a function in a language that does not support it.

Nathann

[vdelecroix]

Replying to ncohen:

It is not sufficient. You were arguing that Python ints were better because faster. I just showed you that it is not completely right. An argument for X is generally not an argument for Y.

All I wanted to point out is that *sometimes* an int is better than an Integer, and that for this reason they should not be forbidden. I do not mean to say that a Python int is always better than a Sage Integer, and quite clearly it is not the case: ints overflows, Integers do not.

Just to be sure: I do not want to forbid Python integer in general. For the output of .cardinality() I consider that Sage integers are *always* better. Perhaps I am wrong and this can be debated if needed. And, as I already mentioned, you can get Python integer for less effort doing len(my_set).

The reason to oblige people to return Sage integers or Infinity in the method .cardinality of a Parent is mostly for consistency.

Couldn't we just use the TestSuite to detect errors?

That is my goal. But we do not agree that returning an int for .cardinality() is an error. In many situation, "ensuring" that it is a Sage integer prevent doing wrong things afterwards.

When you see a non-integer then something is most probably wrong and the testsuite should report it. What you are doing right now is simulate a "return type" for a function in a language that does not support it.

Yes. But having as dependency a ticket "switch from Python to a strongly typed language" would be too much effort. So Python is a fact and we have to deal with it.

Let me mention that Python is trying to do some type checking because it is useful:

sage: class A(object):
....:     def __len__(self):
....:         return 'haha! well tried!'
sage: a = A()
sage: len(a)
Traceback (most recent call last):
...
TypeError: an integer is required

(note that a.__len__() returns what you asked it to do).

Vincent

[git]

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

​d2e0608	Trac 18159: fix the three parents
​a5d1e0e	Trac 18159: some more cleanup in debruijn_sequence.pyx

[jpflori]

I'd say we should ask for others' feeling on sage-devel before making a move here. Personally I don't mind returning a Sage integer for cardinality, but Nathann does not agree. So we should try to satisfy a large proportion of Sage users (or let Sage rot forever).

[bruno]

I agree with Vincent's view here: Having .cardinality() return a Sage Integer is better to my mind since then it comes with all the methods for integers. For a basic user, I guess it may be misleading than for some (finite...) sets you can write S.cardinality().factor() while it sometimes results in an error.

I understand Nathann's concern on efficiency. Yet, my impression is that the issue is going to arise to advanced or expert users (whatever this means). These users should be capable of switching to len(S) if they feel the need for it. The proposed change actually is an improvement since it provides the user the choice for the return type, while no such choice existed before.

[ncohen]

I understand Nathann's concern on efficiency. Yet, my impression is that the issue is going to arise to advanced or expert users (whatever this means). These users should be capable of switching to len(S) if they feel the need for it. The proposed change actually is an improvement since it provides the user the choice for the return type, while no such choice existed before.

The arguments that I see on this thread are not about whether "cardinality" should return python ints or Sage integers.

While the ticket is indeed about .cardinality only, you will see that the arguments given here can be applied to any other Sage function that returns integers.

Of course, having *all* such Sage functions return Integer objects is almost impossible to enforce, yet there is an easy way to enforce that for .cardinality, and this is what the branch implements.

I do not like what is going on here, but I will not oppose it. You are too many.

Nathann

[bruno]

The arguments that I see on this thread are not about whether "cardinality" should return python ints or Sage integers.

While the ticket is indeed about .cardinality only, you will see that the arguments given here can be applied to any other Sage function that returns integers.

That is not totally exact, since as Vincent mentioned it, you can replace S.cardinality() by len(S) if you want an int, and for efficiency concerns, len(S.set()) is the fastest way to get it. Not all functions have this python counterpart.

As a user, I very much cares about the consistency in the return types. And I find this very annoying:

sage: S = Set(range(2^20))
sage: T = CartesianProduct(S,S)
sage: T.cardinality().factor()
2^40
sage: S.cardinality().factor()
Traceback (most recent call last):
...
AttributeError: 'int' object has no attribute 'factor'

Note that I still agree that the same argument may be used at some other places. For instance, let me consider univariate polynomials in sparse representation. I find the following very weird, and would be in favor of changing this behavior:

sage: R.<x> = PolynomialRing(ZZ, sparse=True)
sage: p = x^(2^100)+1
sage: map(lambda t: type(t), p.exponents())
[<type 'int'>, <type 'sage.rings.integer.Integer'>]

I do not like what is going on here, but I will not oppose it. You are too many.

I've understood that (one of) your concern(s) is efficiency issues. But is it the only one? Don't you think that inconsistency also is a problem?

[ncohen]

Please, do whatever you want. I voiced my opinion one thousand times already.

Nathann

[git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

​2d83910	Trac #18159: add a ._test_cardinality in Sets category
​001e781	Trac #18159: remove a duplicated test
​aa0fe00	Trac #18159: fix cardinality/is_finite in sets/set.py
​78f1163	Trac #18159: fix some tests related to TestSuite
​842a3fe	Trac #18159: fix the three parents
​b79143d	Trac #18159: some more cleanup in debruijn_sequence.pyx

[jpflori]

There are conflicts in src/sage/combinat/crystals/monomial_crystals.py.

[vdelecroix]

I will not keep it up to date because you like me working. As you can notice, I rebased it 5 weeks ago and nobody cared. Does your message mean that you will review it? There is no need to keep the branch up to date if there is no reviewer... needs_review means needs a reviewer. Not a patchbot log reader.

Vincent

[jpflori]

I have some time to review it right now. I'm not making you work for fun... it's just that my dev time is very scarce these days. And I have no idea of how to fix the second conflict in the file.

[git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

​575b264	Trac #18159: add a ._test_cardinality in Sets category
​cc87bd5	Trac #18159: remove a duplicated test
​32c403d	Trac #18159: fix cardinality/is_finite in sets/set.py
​937ebf4	Trac #18159: fix some tests related to TestSuite
​eb50b94	Trac #18159: fix two parents
​0aad2f9	Trac #18159: some more cleanup in debruijn_sequence.pyx

[vdelecroix]

Rebased on 6.8.beta6

[jpflori]

Is there any reason why the DeBruijn? squence stuff is included here?

[vdelecroix]

Replying to jpflori:

Is there any reason why the DeBruijn? squence stuff is included here?

Not really. You can ignore 0aad2f9 and I will remove it.

[git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

[jpflori]

Thanks for removing the unrelated commit. LGTM.