#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 )
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)
Change History (10)
Changed 10 years ago by
comment:1 Changed 10 years ago by
- Description modified (diff)
comment:2 Changed 10 years ago by
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
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: ↓ 5 Changed 10 years ago by
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
- 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
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
- 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
- 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
- Cc changed from sage-combinat,wdjoyner@gmail.com to sage-combinat, wdjoyner@gmail.com
- Report Upstream set to N/A
matrix_group_gap-cardinality_len-6250-nt.2.patch superceedes the previous one (should have replaced it)
#4326 depends on it