#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)
Change History (13)
Changed 12 years ago by
comment:1 Changed 12 years ago by
- 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
comment:3 Changed 12 years ago by
This is just an ordering issue (which is arbitrary), the doctest can be changed.
comment:4 Changed 12 years ago by
- 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
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
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
comment:7 Changed 12 years ago by
- 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
I had a trivial reject for Robert's patch which I resolved manually. See
Cheers,
Michael
comment:9 Changed 12 years ago by
- Resolution set to fixed
- Status changed from assigned to closed
Merged in Sage 2.10.1.rc2
Changed 12 years ago by
comment:10 Changed 12 years ago by
[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
I get the following error after the patch: