Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Standardize MatrixGroup_gap: by adding .cardinality, and deprecating __len__ #6250

Closed
nthiery opened this issue Jun 8, 2009 · 13 comments
Closed

Comments

@nthiery
Copy link
Contributor

nthiery commented Jun 8, 2009

Followup on #5308:

  • cardinality now returns the size of the group (was order)
  • order is a backward compatibility alias for cardinality
  • __len__ raises a deprecation error

CC: @sagetrac-sage-combinat @wdjoyner

Component: algebra

Keywords: cardinality, len, order

Author: Nicolas M. Thiéry

Reviewer: David Joyner

Merged: 4.0.2.alpha0

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

@nthiery nthiery added this to the sage-4.0.2 milestone Jun 8, 2009
@nthiery nthiery self-assigned this Jun 8, 2009
@nthiery
Copy link
Contributor Author

nthiery commented Jun 8, 2009

@nthiery
Copy link
Contributor Author

nthiery commented Jun 8, 2009

comment:1

matrix_group_gap-cardinality_len-6250-nt.2.patch superceedes the previous one (should have replaced it)

#4326 depends on it

@nthiery

This comment has been minimized.

@wdjoyner
Copy link

wdjoyner commented Jun 8, 2009

comment:2

I have not applied or tested this patch but upon reading the code a few lines struck me as odd. Can you please explain how, if F is a finite field, F.order() -> F.cardinality() is correct? Has the order method for finite fields been rewritten? Did I miss the discussion on sage-delev that order should be deprecated and replaced by cardinality?

@nthiery
Copy link
Contributor Author

nthiery commented Jun 8, 2009

comment:3

As far as I remember, what was discussed on the list was about the order of an element of a field/group/...

Here this is about the order of the group itself, which is its cardinality.

@wdjoyner
Copy link

comment:4

I don't agree with the suggestion in one of the docstrings that order might be deprecated. But that is just my (American) opinion, which might not be shared by the rest of the world:-) In any case, the patches do not apply cleanly to 4.0.rc0.

@nthiery
Copy link
Contributor Author

nthiery commented Jun 10, 2009

comment:5

Replying to @wdjoyner:

I don't agree with the suggestion in one of the docstrings that order might be deprecated.
But that is just my (American) opinion, which might not be shared by the rest of the world:-)

I am fine with both options. From discussions on sage-devel, it seems that in general aliases are somewhat frowned upon.
We definitely want .cardinality(). But yes, even in Europe, some users would certainly be trying G.order() to get the size
of the group. That's why I raised the issue.

I am happy to remove the comment if you think its better.

In any case, the patches do not apply cleanly to 4.0.rc0.

? I just retried, and it applies smoothly on sage 4.0.1. Did you only apply the second patch? (the first one should be deleted)

@wdjoyner
Copy link

comment:6

The second patch applied cleanly to 4.0.rc0 but failed sage -testall. I think it was unrelated but I'll retry on another version of Sage.

@wdjoyner
Copy link

comment:7

The second patch applied cleanly to 4.0.1.a0 but failed sage -testall. However, that failure (in "devel/sage/sage/misc/html.py") is a known unrelated failure. The patch reads fine and does as it claims.

@ncalexan
Copy link
Mannequin

ncalexan mannequin commented Jun 13, 2009

Changed author from nthiery to Nicolas Thiery

@ncalexan
Copy link
Mannequin

ncalexan mannequin commented Jun 13, 2009

Reviewer: David Joyner

@ncalexan
Copy link
Mannequin

ncalexan mannequin commented Jun 13, 2009

Merged: 4.0.2.alpha0

@ncalexan ncalexan mannequin removed the s: positive review label Jun 13, 2009
@ncalexan ncalexan mannequin closed this as completed Jun 13, 2009
@fchapoton
Copy link
Contributor

Changed author from Nicolas Thiery to Nicolas M. Thiéry

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants