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 niles)

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)

trac_10041_doctest_failure_differential_forms.patch (1.5 KB) - added by jvkersch 9 years ago.

Download all attachments as: .zip

Change History (18)

comment:1 Changed 9 years ago by mpatel

  • Description modified (diff)

comment:2 Changed 9 years ago by jvkersch

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.

./sage -t -long "devel/sage/sage/tensor/differential_forms.py"
sage -t -long "devel/sage/sage/tensor/differential_forms.py"
	 [4.8 s]
 
----------------------------------------------------------------------
All tests passed!
Total time for all tests: 4.8 seconds

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

comment:3 Changed 9 years ago by jason

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 jason

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: Changed 9 years ago by 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.

Time permitting, I will build Sage again on my Linux machine to make sure that the issue is resolved.

comment:6 Changed 9 years ago by jhpalmieri

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: Changed 9 years ago by jason

  • 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 jvkersch

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 drkirkby

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 cremona

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: Changed 9 years ago by jvkersch

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: Changed 9 years ago by niles

  • 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 niles

  • Description modified (diff)

comment:14 in reply to: ↑ 12 Changed 9 years ago by niles

  • 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 leif

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 mpatel

  • Authors set to Joris Vankerschaver

comment:17 Changed 9 years ago by mpatel

  • Merged in set to sage-4.6.rc0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.