#29624 closed defect (fixed)

PermutationGroup_generic.__richcmp__ does not check for op_NE

Reported by: heluani Owned by:
Priority: major Milestone: sage-9.1
Component: group theory Keywords: richcmp, permutationgroup
Cc: Merged in:
Authors: Reimundo Heluani Reviewers: Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: 5f002f1 (Commits, GitHub, GitLab) Commit: 5f002f1ae900cbdec18c169e1e92b31f372b17ad
Dependencies: Stopgaps:

Status badges

Description

Fix PermutationGroup_generic.richcmp usage of op_NE

It only checks for op_EQ now, just added op_NE.

This was reported in sage-devel

https://groups.google.com/forum/#!topic/sage-devel/FlhPCE8XVtY

Change History (11)

comment:1 Changed 18 months ago by vdelecroix

Could you add a doctest that actually checks that the problem is fixed (including a reference to the ticket). Something like

We check that :trac:`29624` is fixed::

    sage: G1 = MyPermGroup1()
    sage: G2 = MyPermGroup2()
    sage: G3 = MyPermGroup3()
    sage: G1 != G2
    X
    sage: G1 != G3
    Y

This would better inside a TESTS section.

comment:2 Changed 18 months ago by git

  • Commit changed from dbd2cece1d1e268b254d0f6d2019f389f82d0a19 to e5fca92f56f8aa3fee9a4496659b7d773cf09448

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

e5fca92added a doctest for 29624

comment:3 Changed 18 months ago by vdelecroix

  • Reviewers set to Vincent Delecroix

The syntax is not correct

TESTS:    <--- only one colon

Check that :trac:`29624` is fixed::   <--- here no indentation and two colons in order to delimit a block code
                                      <--- line break
    sage: G = SymmetricGroup(2)
    sage: H = PermutationGroup([(1,2)])
    sage: not G == H
    False
    sage: G != H
    False

Once this is fixed, you could set the ticket in needs_review so that it can be tested by patchbots.

comment:4 Changed 18 months ago by heluani

I'll make those changes immediately, but I didn't find something explicitly about this on the guide and I checked three things before pushing: 1) The doc build html did not include the test block. 2) The test pass by sage -t and 3) the tests do not pass if I change a FALSE to a TRUE.

comment:5 Changed 18 months ago by git

  • Commit changed from e5fca92f56f8aa3fee9a4496659b7d773cf09448 to 5f002f1ae900cbdec18c169e1e92b31f372b17ad

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

5f002f1fixed indentation on docstring

comment:6 Changed 18 months ago by heluani

  • Status changed from new to needs_review

New commits:

5f002f1fixed indentation on docstring

comment:7 follow-up: Changed 18 months ago by vdelecroix

This is about Rest documentation syntax (mostly disjoint from sage doctesting with sage -t). With your version, the doc would not have compiled correctly. You could look at "building the doc" in the developer guide.

comment:8 in reply to: ↑ 7 ; follow-up: Changed 18 months ago by heluani

I apologize for using this as a discussion, but I'm new here and it's the second time I see something like this so I better understand it. I did build the doc before to make sure that it did fine without an error as per https://doc.sagemath.org/html/en/developer/sage_manuals.html#building-the-manuals

That's why I pushed anyway.

comment:9 in reply to: ↑ 8 Changed 18 months ago by vdelecroix

Replying to heluani:

I apologize for using this as a discussion, but I'm new here and it's the second time I see something like this so I better understand it. I did build the doc before to make sure that it did fine without an error as per https://doc.sagemath.org/html/en/developer/sage_manuals.html#building-the-manuals

That's why I pushed anyway.

Always better to ask questions! And there is also someting that I don't quite know.

Here the situation is a bit weird. The TESTS blocks are excluded from the documentation because we thought that most users would not want to see that. What I meant is that these blocks are excluded from the final compiled HTML or pdf documents. I am not exactly sure how sphinx process the blocks but it might be the case that even with a syntax error it just builds fine.

comment:10 Changed 18 months ago by vdelecroix

  • Status changed from needs_review to positive_review

Thanks for the fix!

comment:11 Changed 18 months ago by vbraun

  • Branch changed from u/heluani/perm_gps_op_ne to 5f002f1ae900cbdec18c169e1e92b31f372b17ad
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.