Opened 2 years ago

Closed 2 years ago

#23930 closed defect (fixed)

Doctest failures in Judson's abstract algebra book

Reported by: mderickx Owned by:
Priority: blocker Milestone: sage-8.1
Component: doctest framework Keywords:
Cc: rbeezer, vdelecroix Merged in:
Authors: Maarten Derickx Reviewers: Vincent Delecroix
Report Upstream: N/A Work issues:
Branch: e633e06 (Commits) Commit: e633e065cf33be46dd653b2ef9da04cfada03c23
Dependencies: Stopgaps:

Description

See the logs of arando

sage -t --long src/sage/tests/books/judson-abstract-algebra/actions-sage.py
**********************************************************************
File "src/sage/tests/books/judson-abstract-algebra/actions-sage.py", line 115, in sage.tests.books.judson-abstract-algebra.actions-sage
Failed example:
    A.orbits()
Expected:
    [['000', '001', '010', '100', '011', '101', '110', '111']]
Got:
    [['010', '011', '001', '100', '101', '000', '110', '111']]
**********************************************************************
File "src/sage/tests/books/judson-abstract-algebra/actions-sage.py", line 126, in sage.tests.books.judson-abstract-algebra.actions-sage
Failed example:
    S.list()
Expected:
    [(),
     ('001','100','010')('011','101','110'),
     ('010','100')('011','101'),
     ('001','010','100')('011','110','101'),
     ('001','100')('011','110'),
     ('001','010')('101','110')]
Got:
    [(),
     ('010','100')('011','101'),
     ('010','100','001')('011','110','101'),
     ('011','110')('001','100'),
     ('010','001','100')('011','101','110'),
     ('010','001')('110','101')]
**********************************************************************
1 item had failures:
   2 of  35 in sage.tests.books.judson-abstract-algebra.actions-sage
    [34 tests, 2 failures, 0.52 s]
------------------------------------

Since this is a machine with a lot of optional packages it is likely that it is caused by that. The results are mathematically correct, it is just that the order of the output changed.

Change History (8)

comment:1 Changed 2 years ago by mderickx

This is now #23930

comment:2 Changed 2 years ago by rbeezer

I'm reluctant to wrap output in sorted() or similar in the textbook, it won't help with understanding. But I think I can rework the testing, perhaps including a hidden test of correctness that a reader won't see.

Maarten - might it be best to mark these random for now, and I'll get proper replacements into the book on the next pass? The diffs in the Sage source will help me remember. This is the first test of how to handle when these fail, so let me know if you can think of a better approach.

I can review a change, but in less than a week I will soon be offline for an extended period.

comment:3 Changed 2 years ago by mderickx

  • Branch set to public/23930
  • Commit set to e633e065cf33be46dd653b2ef9da04cfada03c23

Hi Rob,

Marking as random till the next version of the book works for me. How are the files generated from the book?


New commits:

e633e06Trac #23930: Fixed Doctest failures in Judson's abstract algebra book
Last edited 2 years ago by mderickx (previous) (diff)

comment:4 Changed 2 years ago by mderickx

  • Authors set to Maarten Derickx
  • Status changed from new to needs_review

comment:5 Changed 2 years ago by mderickx

Note that for the A.orbits() test sorting will work. The S.list() test is more complicated since there the same cycles are also notated in different ways.

comment:6 Changed 2 years ago by vdelecroix

  • Reviewers set to Vincent Delecroix
  • Status changed from needs_review to positive_review

The fix is good and non intrusive. It is now up to the book authors to find a more appropriate correction (if any is needed).

comment:7 Changed 2 years ago by rbeezer

Thanks, Vincent. I ran tests overnight, so you just beat me to it on this one! I know it didn't really need a full run of the test suite, but I needed to practice my nascent git skills with the full Sage workflow. ;-)

I'll likely clean up all the tests in the book again right around the end of the year, and get this cleaned up properly then.

Thanks, Maarten, also for shepherding this one through.

comment:8 Changed 2 years ago by vbraun

  • Branch changed from public/23930 to e633e065cf33be46dd653b2ef9da04cfada03c23
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.