Opened 10 years ago

Closed 10 years ago

Last modified 3 years ago

#6250 closed defect (fixed)

[with patch, positive review] Standardize MatrixGroup_gap: by adding .cardinality, and deprecating __len__

Reported by: nthiery Owned by: nthiery
Priority: major Milestone: sage-4.0.2
Component: algebra Keywords: cardinality, __len__, order, groups
Cc: sage-combinat, wdjoyner@… Merged in: 4.0.2.alpha0
Authors: Nicolas M. Thiéry Reviewers: David Joyner
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by nthiery)

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

Attachments (1)

matrix_group_gap-cardinality_len-6250-nt.2.patch (7.1 KB) - added by nthiery 10 years ago.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 10 years ago by nthiery

  • Description modified (diff)

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

#4326 depends on it

comment:2 Changed 10 years ago by wdj

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?

comment:3 Changed 10 years ago by nthiery

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.

comment:4 follow-up: Changed 10 years ago by wdj

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.

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

  • Status changed from new to assigned

Replying to wdj:

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)

comment:6 Changed 10 years ago by wdj

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.

comment:7 Changed 10 years ago by wdj

  • Summary changed from [with patch, needs review] Standardize MatrixGroup_gap: by adding .cardinality, and deprecating __len__ to [with patch, positive review] Standardize MatrixGroup_gap: by adding .cardinality, and deprecating __len__

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.

comment:8 Changed 10 years ago by ncalexan

  • Authors changed from nthiery to Nicolas Thiery
  • Merged in set to 4.0.2.alpha0
  • Resolution set to fixed
  • Reviewers set to David Joyner
  • Status changed from assigned to closed

comment:9 Changed 3 years ago by chapoton

  • Authors changed from Nicolas Thiery to Nicolas M. Thiéry
  • Cc changed from sage-combinat,wdjoyner@gmail.com to sage-combinat, wdjoyner@gmail.com
  • Report Upstream set to N/A
Note: See TracTickets for help on using tickets.