Opened 9 years ago

Closed 6 years ago

#10093 closed enhancement (fixed)

clean up documentation of sage/misc/bitset.pyx

Reported by: mvngu Owned by: mvngu
Priority: major Milestone: sage-5.13
Component: documentation Keywords:
Cc: jason Merged in: sage-5.13.rc0
Authors: Minh Van Nguyen, Jeroen Demeyer Reviewers: Nathann Cohen, David Coudert
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

As the subject says. This ticket is part of the larger project at #7656.

Apply:

Attachments (4)

trac-10093_cleanup-doc.patch (77.0 KB) - added by mvngu 9 years ago.
trac-10093_cleanup-doc-v2.patch (77.1 KB) - added by mvngu 9 years ago.
trac-10093_cleanup-doc-v3.patch (77.1 KB) - added by jdemeyer 6 years ago.
10093_empty.patch (7.7 KB) - added by jdemeyer 6 years ago.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 9 years ago by mvngu

  • Description modified (diff)
  • Milestone set to sage-4.6.1

comment:2 Changed 9 years ago by jason

  • Cc jason added

Changed 9 years ago by mvngu

comment:3 Changed 9 years ago by mvngu

  • Authors set to Minh Van Nguyen
  • Description modified (diff)
  • Status changed from new to needs_review

The attached patch is a massive clean-up of the documentation and doctests in the module sage/misc/bitset.pyx. Here's a summary of what's in the patch:

  • Add the title "Bitset" as the title of the module. When generating the documentation with Sphinx, this name would become the title of the module in the generated documentation.
  • Many typo fixes and remove trailing white spaces.
  • Format code according to PEP 008 wherever possible.
  • Raise exceptions according to the style of Python 3.x.
  • Add lots more doctests to the class documentation of FrozenBitset.
  • Move input, output documentation from __init__ in FrozenBitset to the class documentation. This should make the documentation for input and output appear in the reference manual.
  • Cross reference FrozenBitset and Bitset; provide link to Python's set types in the Python library reference.
  • Explain the string representation of FrozenBitset and Bitset.
  • Get issubset, issuperset and isdisjoint to propagate exceptions; previously those methods ignored any exceptions raised.
  • Illustrate difference between length and capacity of a bitset.
  • For FrozenBitset, doctests for None type in comparison, containment, union, or, intersection, and, difference, symmetric difference, xor.
  • For Bitset, tests for None type in update, intersection_update, difference_update, symmetric_difference_update; tests for invalid input to add, remove, discard.

comment:4 follow-up: Changed 9 years ago by jason

Note that this example: sage: FrozenBitset(()) does not contain a tuple (though it looks like it does...

Changed 9 years ago by mvngu

comment:5 in reply to: ↑ 4 Changed 9 years ago by mvngu

  • Description modified (diff)

Replying to jason:

Note that this example: sage: FrozenBitset(()) does not contain a tuple (though it looks like it does...

I'm not sure what it is that you want me to modify in my patch. Do you want me to remove it? Do you want me to make some clarification in the document? The purpose of the block within which that example is found is to demonstrate different ways of constructing the empty bitset. If you want some more clarification, I have added the following examples to my patch:

  • FrozenBitet(tuple())
  • FrozenBitet(list())

See the ticket description for instruction on which patch to apply.

Also note that in the current implementation of FrozenBitset, the constructor doesn't accept the empty string. Passing the empty string to the constructor of FrozenBitset would result in a MemoryError. This is fixed in both of my patches. My intention is to say that passing the empty string to the constructor of FrozenBitset would result in the empty bitset.

comment:6 Changed 9 years ago by jason

Sorry, I should have been clearer. I read through most of the patch, and just noticed that one thing that looked confusing.

Thanks for fixing the empty string issue, and the huge number of other issues, and for formatting the docs so nicely!

comment:7 follow-up: Changed 9 years ago by ncohen

  • Status changed from needs_review to needs_work

Wow, great patch ! Thank you Minh ! And thanks to whoever wrote this wonderful bitset class in the first place ! I'm making great use of it these days :-)

Just two things :

  • First, the patch does not apply on rc1, but that's just because of 2 short documentation lines
  • In the documentation the variable "iter" represents an iterable from which the FrozenBitset? can be created... In Python's terminology, is an iterator an iterable ? Because in this case checking its length may be a bad idea... And converting it to a list too perhaps. Well, short of these two things, this patch is good to go (and great, being Minh's work)

Nathann

comment:8 in reply to: ↑ 7 Changed 8 years ago by mvngu

  • Description modified (diff)
  • Status changed from needs_work to needs_review

Replying to ncohen:

Just two things :

  • First, the patch does not apply on rc1, but that's just because of 2 short documentation lines

This should be fixed now with trac-10093_cleanup-doc-v3.patch

  • In the documentation the variable "iter" represents an iterable from which the FrozenBitset? can be created... In Python's terminology, is an iterator an iterable ? Because in this case checking its length may be a bad idea... And converting it to a list too perhaps. Well, short of these two things, this patch is good to go (and great, being Minh's work)

The variable "iter" is meant to signify that the constructor expects an iterator, something whose elements we can step through one element at a time. To do the stepping, we need an iterator: a function or something that allows us to do the stepping through from start to finish. So an iterable is different from an iterator.

comment:9 Changed 8 years ago by ncohen

  • Reviewers set to Nathann Cohen
  • Status changed from needs_review to positive_review

Well, in that case :-)

Nathann

comment:10 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-4.8 to sage-5.0

comment:11 Changed 8 years ago by jdemeyer

  • Status changed from positive_review to needs_work
  • Work issues set to 32-bit

This fails on 32-bit i386 Linux:

$ ./sage -t devel/sage/sage/misc/bitset.pyx
sage -t  "devel/sage/sage/misc/bitset.pyx"
**********************************************************************
File "/home/jdemeyer/sage-5.0.beta0/devel/sage/sage/misc/bitset.pyx", line 238:
    sage: FrozenBitset("", capacity=0)
Expected:
    Traceback (most recent call last):
    ...
    MemoryError...
Got:
    <BLANKLINE>
**********************************************************************
File "/home/jdemeyer/sage-5.0.beta0/devel/sage/sage/misc/bitset.pyx", line 245:
    sage: FrozenBitset("110110", capacity=0)
Expected:
    Traceback (most recent call last):
    ...
    MemoryError...
Got:
    Traceback (most recent call last):
      File "/home/jdemeyer/sage-5.0.beta0/local/bin/ncadoctest.py", line 1231, in run_one_test
        self.run_one_example(test, example, filename, compileflags)
      File "/home/jdemeyer/sage-5.0.beta0/local/bin/sagedoctest.py", line 38, in run_one_example
        OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
      File "/home/jdemeyer/sage-5.0.beta0/local/bin/ncadoctest.py", line 1172, in run_one_example
        compileflags, 1) in test.globs
      File "<doctest __main__.example_1[46]>", line 1, in <module>
        FrozenBitset("110110", capacity=Integer(0))###line 245:
    sage: FrozenBitset("110110", capacity=0)
      File "bitset.pyx", line 365, in sage.misc.bitset.FrozenBitset.__init__ (sage/misc/bitset.c:4396)
    ValueError: bitset capacity does not match passed string
**********************************************************************
File "/home/jdemeyer/sage-5.0.beta0/devel/sage/sage/misc/bitset.pyx", line 249:
    sage: FrozenBitset("110110", capacity=-0)
Expected:
    Traceback (most recent call last):
    ...
    MemoryError...
Got:
    Traceback (most recent call last):
      File "/home/jdemeyer/sage-5.0.beta0/local/bin/ncadoctest.py", line 1231, in run_one_test
        self.run_one_example(test, example, filename, compileflags)
      File "/home/jdemeyer/sage-5.0.beta0/local/bin/sagedoctest.py", line 38, in run_one_example
        OrigDocTestRunner.run_one_example(self, test, example, filename, compileflags)
      File "/home/jdemeyer/sage-5.0.beta0/local/bin/ncadoctest.py", line 1172, in run_one_example
        compileflags, 1) in test.globs
      File "<doctest __main__.example_1[47]>", line 1, in <module>
        FrozenBitset("110110", capacity=-Integer(0))###line 249:
    sage: FrozenBitset("110110", capacity=-0)
      File "bitset.pyx", line 365, in sage.misc.bitset.FrozenBitset.__init__ (sage/misc/bitset.c:4396)
    ValueError: bitset capacity does not match passed string
**********************************************************************
1 items had failures:
   3 of  50 in __main__.example_1
***Test Failed*** 3 failures.
For whitespace errors, see the file /home/jdemeyer/.sage//tmp/bitset_11221.py
         [3.7 s]

----------------------------------------------------------------------
The following tests failed:


        sage -t  "devel/sage/sage/misc/bitset.pyx"
Total time for all tests: 3.7 seconds

comment:12 Changed 8 years ago by jdemeyer

The same error on OS X 10.4 PPC 32-bit.

comment:13 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

Changed 6 years ago by jdemeyer

comment:14 Changed 6 years ago by jdemeyer

  • Authors changed from Minh Van Nguyen to Minh Van Nguyen, Jeroen Demeyer
  • Description modified (diff)
  • Status changed from needs_work to needs_review
  • Work issues 32-bit deleted

Rebased the original patch (which still has positive_review I guess) and added an additional patch to deal with the empty bitset.

comment:15 Changed 6 years ago by dcoudert

  • Reviewers changed from Nathann Cohen to Nathann Cohen, David Coudert
  • Status changed from needs_review to positive_review

This patch passes all tests on my computer. So I change the status to positive.

Changed 6 years ago by jdemeyer

comment:16 Changed 6 years ago by jdemeyer

  • Merged in set to sage-5.13.rc0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.