Opened 13 years ago

Closed 3 years ago

Last modified 3 years ago

#5548 closed defect (fixed)

fix that _hnf_mod segfaults sage completely

Reported by: was Owned by: was
Priority: major Milestone: sage-8.8
Component: linear algebra Keywords:
Cc: burcin, jdemeyer Merged in:
Authors: Alex Sun Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 8c43ba0 (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description

teragon:algebras wstein$ sage
----------------------------------------------------------------------
| Sage Version 3.4, Release Date: 2009-03-11                         |
| Type notebook() for the GUI, and license() for information.        |
----------------------------------------------------------------------
sage: a = random_matrix(ZZ,16,4)
sage: a._hnf_mod(100)


------------------------------------------------------------
Unhandled SIGFPE: An unhandled floating point exception occured in SAGE.
This probably occured because a *compiled* component
of SAGE has a bug in it (typically accessing invalid memory)
or is not properly wrapped with _sig_on, _sig_off.
You might want to run SAGE under gdb with 'sage -gdb' to debug this.
SAGE will now terminate (sorry).
------------------------------------------------------------

Change History (19)

comment:1 Changed 13 years ago by was

(the problem is that the input matrix isn't square...)

comment:2 Changed 13 years ago by burcin

  • Cc burcin added

comment:3 Changed 12 years ago by jdemeyer

  • Cc jdemeyer added
  • Report Upstream set to N/A

comment:4 Changed 9 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:5 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:6 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:7 Changed 8 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:8 Changed 3 years ago by gh-black-stones

  • Branch set to u/gh-black-stones/5548
  • Milestone changed from sage-6.4 to sage-8.8

comment:9 Changed 3 years ago by git

  • Commit set to 8c43ba044760d63e7c11188368343cc872458c6a

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

8c43ba0Check matrix is square

comment:10 Changed 3 years ago by gh-black-stones

  • Authors set to Alex Sun
  • Status changed from new to needs_review

Raised a ValueError? if the matrix is not square, and made a doctest out of the sample code in the description.

comment:11 Changed 3 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to positive_review

comment:12 Changed 3 years ago by vbraun

  • Branch changed from u/gh-black-stones/5548 to 8c43ba044760d63e7c11188368343cc872458c6a
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:13 follow-up: Changed 3 years ago by slelievre

  • Commit 8c43ba044760d63e7c11188368343cc872458c6a deleted

Shouldn't the added doctest block start with a TEST:: block header?

comment:14 in reply to: ↑ 13 Changed 3 years ago by gh-black-stones

Replying to slelievre:

Shouldn't the added doctest block start with a TEST:: block header?

I believe that a line ending in :: signifies the code following it is a doctest, it doesn't necessarily need to be TEST::. There are a few exceptions, like how MATH:: indicates latex, but there are many doctests that say something like "This fixes trac X::", followed by a doctest.

comment:15 Changed 3 years ago by slelievre

Although the code does get tested even without a TEST header, my understanding of the documentation strings section of Sage's coding basics is that testable examples in docstrings are usually placed inside "TEST" or "TESTS" or "EXAMPLE" or "EXAMPLES" blocks, which can go either

TESTS::

    sage: 1 + 1
    2
    sage: 1 + 2
    3

or

TESTS:

We test that one plus one is two::

    sage: 1 + 1
    2

We test that one plus two is three::

    sage: 1 + 2
    3

(or similarly for TEST, EXAMPLE, EXAMPLES).

In addition, TEST and TESTS sections being meant more for developers than users, they get removed when building the documentation, unless one sets a dedicate environment variable:

SAGE_DOCBUILD_OPTS="--include-tests-blocks"

to keep them in.

Are there any doctests in docstrings elsewhere in the Sage sources that are not introduced by an EXAMPLE, EXAMPLES, TEST or TESTS header?

comment:16 Changed 3 years ago by jdemeyer

I agree with Samual that there should have been a TESTS: block. I missed that during review. It's not a big problem though...

comment:17 Changed 3 years ago by gh-black-stones

I see, I didn't realize that the TESTS:: header indicated a big block for all the tests, sorry for leaving that out. That being said, I did a quick search and found that there are some (very few) files that don't follow this...

  • set_calculus_method() of sage-master/src/sage/manifolds/manifold.py
  • from_tamari_sorting_tuple() of sage-master/src/sage/combinat/binary_tree.py ("EXEMPLES" instead of "EXAMPLES")
  • init() of GenericGraphQuery? in sage-master/src/sage/graphs/graph_database.py
  • this file

My "quick search" was a bad Python script that has most likely left out other mistakes. Furthermore, some docstrings use "Examples" or "Tests" instead of the all uppercase version, and others have "Examples:" instead of "Examples::". I wasn't sure if they mattered so I didn't include them (also there were a lot of them).

This ticket is already closed, so let's not reopen it. Do you think it would be worth it to make a new ticket to search for and fix all these docstring mistakes?

comment:18 Changed 3 years ago by slelievre

A new ticket dedicated to fixing all missing or non-uppercase EXAMPLES and TESTS block headers would be nice.

comment:19 Changed 3 years ago by jdemeyer

If you want to fix those mistakes, great! Reviewing that should be trivial.

Note: See TracTickets for help on using tickets.