Ticket #3676 (closed enhancement: fixed)

Opened 19 months ago

Last modified 18 months ago

[with patch, positive review] Refactor graph isom code.

Reported by: rlm Owned by: rlm
Priority: major Milestone: sage-3.1
Component: combinatorics Keywords: graphs
Cc: boothby Author(s):
Report Upstream: Reviewer(s):
Merged in: Work issues:

Description

After this patch, graph_isom will be essentially obsolete. Brought to the GNU General Public by Google, Inc.

Attachments

trac3676-refactor_graph_isom.patch Download (79.4 KB) - added by rlm 19 months ago.
trac3676-bitset_pxd.patch Download (3.8 KB) - added by rlm 19 months ago.
trac3676-cleanup.patch Download (3.4 KB) - added by rlm 18 months ago.
trac3676-cleanup2.patch Download (3.9 KB) - added by rlm 18 months ago.
trac3676-cleanup3.patch Download (1.2 KB) - added by rlm 18 months ago.
3676-ncalexan-docstring-changes.patch Download (4.8 KB) - added by ncalexan 18 months ago.
trac_3676-graph_isom_refactor.patch Download (88.0 KB) - added by rlm 18 months ago.
Apply only this patch (please do not delete the others!)

Change History

Changed 19 months ago by rlm

Changed 19 months ago by rlm

  • cc boothby added

Changed 19 months ago by wdj

FYI, I got the following test failure:

wdj@hera:~/sagefiles/sage-3.0.4.rc0$ ./sage -t  devel/sage/sage/rings/finite_field_ntl_gf2e.pyx
sage -t  devel/sage/sage/rings/finite_field_ntl_gf2e.pyx    **********************************************************************
File "/home/wdj/sagefiles/sage-3.0.4.rc0/tmp/finite_field_ntl_gf2e.py", line 170:
    sage: k.modulus()
Expected:
    x^17 + x^16 + x^15 + x^10 + x^8 + x^6 + x^4 + x^3 + x^2 + x + 1
Got:
    x^17
**********************************************************************
1 items had failures:
   1 of   7 in __main__.example_2
***Test Failed*** 1 failures.
For whitespace errors, see the file /home/wdj/sagefiles/sage-3.0.4.rc0/tmp/.doctest_finite_field_ntl_gf2e.py
         [2.9 s]
exit code: 1024

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


        sage -t  devel/sage/sage/rings/finite_field_ntl_gf2e.pyx
Total time for all tests: 2.9 seconds

Changed 19 months ago by mabshoff

David,

the above doctest is a known issue and orthogonal to rlm's code. See #3634 for the patch that likely caused this.

Cheers,

Michael

Changed 19 months ago by wdj

Okay. I was just trying to help out with the doctesting, that's all. Seems like it tests fine then.

Changed 19 months ago by rlm

Changed 18 months ago by boothby

It's so... readable...

Changed 18 months ago by rlm

Changed 18 months ago by rlm

The patches here may depend on #3703.

Changed 18 months ago by rlm

Changed 18 months ago by rlm

  • milestone changed from sage-3.1.1 to sage-3.1

Changed 18 months ago by rlm

Changed 18 months ago by rlm

I can flatten those last three if desired...

Changed 18 months ago by mabshoff

Once this ticket is merged #3786 should be next.

Cheers,

Michael

Changed 18 months ago by ncalexan

Changed 18 months ago by ncalexan

  • summary changed from [with patch, needs review] Refactor graph isom code. to [with patch, needs review, review in progress] Refactor graph isom code.

3676-ncalexan-docstring-changes.patch changes some documentation to be clearer.

I am happy with this patch, save for a missing module docstring. rlmiller will write said docstring, explaining programming API to his code, and then this is ready for showtime.

Apply all patches in order.

Changed 18 months ago by boothby

Looks good to me.

Changed 18 months ago by rlm

The last patch is a flattened version of the previous ones, together with a recipe for implementing other objects. It should be finally ready to go. Apply only the last patch.

Changed 18 months ago by rlm

Apply only this patch (please do not delete the others!)

Changed 18 months ago by ncalexan

  • summary changed from [with patch, needs review, review in progress] Refactor graph isom code. to [with patch, positive review] Refactor graph isom code.

rlm and I have gone back and forth on this and I think it's great. I say apply!

Changed 18 months ago by mabshoff

  • status changed from new to closed
  • resolution set to fixed

Merged in Sage 3.1.alpha2

Note: See TracTickets for help on using tickets.