Opened 9 years ago
Closed 9 years ago
#10041 closed defect (fixed)
Doctest failure in sage/tensor/differential_forms.py
Reported by: | mpatel | Owned by: | mvngu |
---|---|---|---|
Priority: | blocker | Milestone: | sage-4.6 |
Component: | doctest coverage | Keywords: | |
Cc: | jason, jvkersch, mhampton, niles | Merged in: | sage-4.6.rc0 |
Authors: | Joris Vankerschaver | Reviewers: | Niles Johnson |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
With the forthcoming 4.6.alpha2, I get this doctest failure on the Skynet machines cicero (x86-Linux-pentium4-fc) and fulvia (x86_64-SunOS-core2):
sage -t -long "devel/sage/sage/tensor/differential_forms.py" ********************************************************************** File "/home/mpatel/build/cicero/sage-4.6.alpha2/devel/sage/sage/tensor/differential_forms.py", line 132: sage: cmp(F, G) Expected: 1 Got: -1
Related: #9650.
Apply
Attachments (1)
Change History (18)
comment:1 Changed 9 years ago by
- Description modified (diff)
comment:2 Changed 9 years ago by
comment:3 Changed 9 years ago by
The problem seems to be in the coordinate patch code, I think.
Notice that:
sage: var('x,y,z') (x, y, z) sage: (x,y,z)<(y,x,z) x < y
I think that comparing tuples of symbolic variables is not well-defined. Instead, you might compare strings (i.e., call str() on each tuple)
comment:4 Changed 9 years ago by
Do you really want to force an order on these things, or would it be enough to only define an equality-checking function?
comment:5 follow-up: ↓ 7 Changed 9 years ago by
Thanks Jason! I am uploading a patch to convert the tuples of coordinates to strings before comparing and I hope this solves the issue. I'm not sure we should ignore the order in coordinate tuples. For instance, T(T*Q) has natural bundle coordinates (q, p, q', p') whereas T*(TQ) naturally has coordinates (q, q', p, p'). The two bundles are isomorphic by switching the relevant coordinates, but it would be good to keep the distinction.
Time permitting, I will build Sage again on my Linux machine to make sure that the issue is resolved.
Changed 9 years ago by
comment:6 Changed 9 years ago by
The failing doctest now passes for me on cicero and fulvia. If someone else can confirm that it passes on some other machines (on ones where it passed before, we should make sure it still passes), feel free to give it a positive review.
Should this ticket be marked as "needs review"?
comment:7 in reply to: ↑ 5 ; follow-up: ↓ 8 Changed 9 years ago by
- Status changed from new to needs_review
Replying to jvkersch:
Thanks Jason! I am uploading a patch to convert the tuples of coordinates to strings before comparing and I hope this solves the issue. I'm not sure we should ignore the order in coordinate tuples. For instance, T(T*Q) has natural bundle coordinates (q, p, q', p') whereas T*(TQ) naturally has coordinates (q, q', p, p'). The two bundles are isomorphic by switching the relevant coordinates, but it would be good to keep the distinction.
I meant, do you want to force a total order on the set of coordinates? In other words, what does (q,q',p,p')<(q,p,q',p') mean? Or do you just want to have equality testing, i.e., you can tell that (q,q',p,p') != (q,p,q',p')?
Regardless, I think resolving that design issue should be a separate ticket.
comment:8 in reply to: ↑ 7 Changed 9 years ago by
Replying to jason:
I meant, do you want to force a total order on the set of coordinates?
Good point -- that wouldn't make sense indeed.
Regardless, I think resolving that design issue should be a separate ticket.
I've put this up for review as #10053. This patch just takes out the calls to the __cmp__
member functions and replaces them with equivalent calls to __eq__
.
comment:9 Changed 9 years ago by
I also see this on OpenSolaris on x86 on the clean 4.6.alpha1 code. I don't feel able to comment on the patches.
sage -t -long devel/sage/sage/tensor/differential_forms.py ********************************************************************** File "/export/home/drkirkby/sage-4.6.alpha2/devel/sage-main/sage/tensor/differential_forms.py", line 132: sage: cmp(F, G) Expected: 1 Got: -1 ********************************************************************** 1 items had failures: 1 of 11 in __main__.example_3 ***Test Failed*** 1 failures. For whitespace errors, see the file /export/home/drkirkby/.sage//tmp/.doctest_differential_forms.py
comment:10 Changed 9 years ago by
The patch works for me (tests in that one file pass, which used to fail); I have not tested the whole library, just sage/tensor/*. This is on 32-bit ubuntu.
comment:11 follow-up: ↓ 12 Changed 9 years ago by
For what it's worth, make ptestlong
tells me that all tests passed on my 64bit Linux machine (CentOS Linux 5.5).
comment:12 in reply to: ↑ 11 ; follow-up: ↓ 14 Changed 9 years ago by
- Reviewers set to Niles Johnson
I applied this patch, and the one at #10053. With these, sage passes all (-long
) doctests (linux, with Red Hat Enterprise). Note that the remaining issue, comment 9 above, appears because this patch addresses comparison only for coordinate patches, not differential form algebras or differential form elements. Since these are addressed by #10053, this ticket can get positive review when that one does.
comment:13 Changed 9 years ago by
- Description modified (diff)
comment:14 in reply to: ↑ 12 Changed 9 years ago by
- Status changed from needs_review to positive_review
Replying to niles:
I applied this patch, and the one at #10053. With these, sage passes all (
-long
) doctests (linux, with Red Hat Enterprise). Note that the remaining issue, comment 9 above, appears because this patch addresses comparison only for coordinate patches, not differential form algebras or differential form elements. Since these are addressed by #10053, this ticket can get positive review when that one does.
#10053 now has positive review.
comment:15 Changed 9 years ago by
Just for the record:
I now get this doctest error after upgrade-testing (#9896) with Sage 4.6.alpha3 (in-place upgrade from 4.5.3) on Ubuntu 9.04 x86, Pentium4 Prescott with SSE enabled (in CFLAGS and CXXFLAGS).
In the build from scratch on the same machine, with the same settings, I didn't get that error.
comment:16 Changed 9 years ago by
comment:17 Changed 9 years ago by
- Merged in set to sage-4.6.rc0
- Resolution set to fixed
- Status changed from positive_review to closed
I will need to think some more about this. After hearing about this bug, I built 4.6.alpha2 on my Mac OS 10.6 machine and ran the doctests there, all of which passed without errors.
I am currently building Sage on my Linux machine and hope to be able to reproduce that bug shortly. In the meantime, could this be due to
cmp
behaving differently on different platforms? This would seem very weird to me...