Opened 15 years ago

Closed 15 years ago

Last modified 15 years ago

#1823 closed defect (fixed)

[with patch, with positive review] RDF/CDF coverage, consistent hashing

Reported by: Robert Bradshaw Owned by: Robert Bradshaw
Priority: major Milestone: sage-2.10.1
Component: basic arithmetic Keywords:
Cc: Merged in:
Authors: Reviewers:
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

Some functions, e.g. one-liners and stuff dealing with the RDF pool, are hard or useless to test in the doctest setting.

Hashing is now consistent between RDF, RR, CDF, CC, float, and complex.

Attachments (3)

rdf-cdf-coverage.diff (22.6 KB) - added by Robert Bradshaw 15 years ago.
rdf-cdf-fixup.patch (3.5 KB) - added by Carl Witty 15 years ago.
Sage-2.10.1.rc2-add-autogenerated-files-to-.hgignore.patch (752 bytes) - added by Michael Abshoff 15 years ago.

Download all attachments as: .zip

Change History (13)

Changed 15 years ago by Robert Bradshaw

Attachment: rdf-cdf-coverage.diff added

comment:1 Changed 15 years ago by Robert Bradshaw

Milestone: sage-2.10.1
Owner: changed from somebody to Robert Bradshaw
Status: newassigned

comment:2 Changed 15 years ago by Mike Hansen

I get the following error after the patch:

    sage: whole, parts = q.partial_fraction_decomposition(); parts
Expected:
    [(-7.6294e-6*x^2 + 1.0000)/(1.0000*x^4 + 4.0000*x^2 + 4.0000), 1.0000/(1.0000*x - 1.0000)]
Got:
    [1.0000/(1.0000*x - 1.0000), (-7.6294e-6*x^2 + 1.0000)/(1.0000*x^4 + 4.0000*x^2 + 4.0000)]

comment:3 Changed 15 years ago by Robert Bradshaw

This is just an ordering issue (which is arbitrary), the doctest can be changed.

comment:4 Changed 15 years ago by ncalexan

Summary: [with patch] RDF/CDF coverage, consistent hashing[with patch, with mostly positive review] RDF/CDF coverage, consistent hashing

Patch looks excellent. I would like the int(...) and long(...) functions to actually doctest that their output has the expected type, but that's a small issue.

I cannot speak to whether the hashing strategy is a good design decision.

comment:5 Changed 15 years ago by ncalexan

robertwb claims "Hashing is now consistent between RDF, RR, CDF, CC, float, and complex."

Is this doctested? I don't think so. It should be. Should this maybe go in the module level tests for RDF, CDF, or something like that?

comment:6 Changed 15 years ago by Robert Bradshaw

Yes, the consistency between RDF, RR, CDF, CC, float, and complex is doctested, the doctests for the __hash__ functions each test against hashing the native Python type.

Changed 15 years ago by Carl Witty

Attachment: rdf-cdf-fixup.patch added

comment:7 Changed 15 years ago by Carl Witty

Summary: [with patch, with mostly positive review] RDF/CDF coverage, consistent hashing[with patch, with positive review] RDF/CDF coverage, consistent hashing

Once both rdf-cdf-coverage.diff and rdf-cdf-fixup.patch are applied, all doctests pass on both 32-bit and 64-bit x86 Linux.

The code looks good.

comment:8 Changed 15 years ago by Michael Abshoff

I had a trivial reject for Robert's patch which I resolved manually. See

http://sage.math.washington.edu/home/mabshoff/release-cycles-2.10.1/rc2/trac_1823_rdf-cdf-coverage-missing-hunk.patch

Cheers,

Michael

comment:9 Changed 15 years ago by Michael Abshoff

Resolution: fixed
Status: assignedclosed

Merged in Sage 2.10.1.rc2

comment:10 Changed 15 years ago by Michael Abshoff

[23:19] <mabshoff> cwitty: after applying #1823 I have a comple_double.h and complex_double_api.h 
[23:20] <mabshoff> in sage/rings all the sudden.
[23:20] <mabshoff> And hg claims they are not under revision control. Any idea what is up with that?
[23:22] <cwitty> mabshoff, strange Cython magic?
[23:23] <mabshoff> No clue. I grepped the tree for any file including them and there are none.
[23:23] <cwitty> They're clearly autogenerated, and are probably caused by this:
[23:23] <cwitty> cdef public api ComplexDoubleElement new_ComplexDoubleElement()
[23:23] <cwitty> from Robert's patch.
[23:23] <mabshoff> ok
[23:23] <cwitty> But I don't know why Robert thought that should be public.
[23:24] <mabshoff> Maybe something that he will follow up with?
[23:24] <mabshoff> But what to do? Put them into the repo? I don't think we will get an answer today.
[23:25] <cwitty> No; I think add them to .hgignore.
[23:25] <cwitty> (Auto-generated files shouldn't go in the repository.)

Ad per cwitty suggestion I added the to .hgignore. The patch attached above has one extra, erroneous change (sage->SAGE [damn vi]), that I reverted already in my repo. It is just attached for the record. If adding those two files to .hgignore was a mistake please let us know.

Cheers,

Michael

Note: See TracTickets for help on using tickets.