Opened 12 years ago

Closed 12 years ago

Last modified 12 years ago

#1823 closed defect (fixed)

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

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

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 robertwb 12 years ago.
rdf-cdf-fixup.patch (3.5 KB) - added by cwitty 12 years ago.
Sage-2.10.1.rc2-add-autogenerated-files-to-.hgignore.patch (752 bytes) - added by mabshoff 12 years ago.

Download all attachments as: .zip

Change History (13)

Changed 12 years ago by robertwb

comment:1 Changed 12 years ago by robertwb

  • Milestone set to sage-2.10.1
  • Owner changed from somebody to robertwb
  • Status changed from new to assigned

comment:2 Changed 12 years ago by mhansen

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 12 years ago by robertwb

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

comment:4 Changed 12 years ago by ncalexan

  • Summary changed from [with patch] RDF/CDF coverage, consistent hashing to [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 12 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 12 years ago by robertwb

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 12 years ago by cwitty

comment:7 Changed 12 years ago by cwitty

  • Summary changed from [with patch, with mostly positive review] RDF/CDF coverage, consistent hashing to [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 12 years ago by mabshoff

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 12 years ago by mabshoff

  • Resolution set to fixed
  • Status changed from assigned to closed

Merged in Sage 2.10.1.rc2

comment:10 Changed 12 years ago by mabshoff

[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.